-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Variables and experimental features. #2116
base: dev/33-serpierite
Are you sure you want to change the base?
Variables and experimental features. #2116
Conversation
…ssue/554c-variables
…ues are calculated on the server and sent over
…he first use of a variable in text.
…ages. Fixed variables owned by other nodes reading as 'undefined' in the viewer when used. Need to add variables to all node converters, add variable name validation and fix tests.
…invalid variables. Overhauled variable creation/validation in the visual editor to supply reasonable defaults when creating new variables or changing variables from one type to another, and added relevant logic to display messages when there are errors present and highlight relevant inputs. Moved some CSS rules into new files closer to the components they're styling.
…to enable variable substitution in the nav area.
…parsers to add variable support. Created migration to add a 'state' column to visits for tracking variable values per visit, for use in tracking those same generated values when starting an assessment attempt. Added support for variable usage in assessment attempt and review contexts.
…up unused files and/or blocks of code. Cleaned up/clarified files per findings from tests.
This is just about done and working, but there are a couple of potentially serious problems that we probably need to address before we can ship this. These problems only really surface when using the Paging @zachberry for any ideas for approaching the things listed in 'Problems' in the PR description above. Currently it doesn't look like there is any way of circumventing the way Obojobo handles questions in assessment attempts and during assessment review in order to make the questions' full node ancestor hierarchies available to the variable util. I'm considering trying to find some way of attaching each question's full ancestor tree on the server side and then somehow using that information on the viewer side, but even in the best case scenario the solution is going to be extremely hacky. |
I probably need to re-familiarize myself a bit more, but some initial top-of-my-head thoughts:
The server could return the following when requesting a document (I'm not tied to this exact structure, just trying to convey the idea): {
root: { //...the whole obo document...// },
variables: [
{
name: "studentName",
value: "Jane",
owner: "p1",
refs: ["t1"]
}
]
} So the server comes back with a variable value it chose (Jane), where the variable is defined (the "owner", which is "p1", the Page) and finally anywhere the variable is used (the "refs", which is "t1", the TextChunk that uses it). (You would ideally want to further process the const variableValuesByOboNodeId = {
p1: {
studentName: "Jane"
},
t1: {
studentName: "Jane"
}
} That way when the parser/viewer comes to If this was done for everything in the document then you would be fine if you're missing part of the node structure, such as in Assessment, because that Thoughts? I might be missing something obvious since it's been a minute since I looked at this stuff. |
My knee-jerk reaction to performing variable substitutions server-side but just for assessments is that we may as well just perform substitutions server-side for the whole document. There was a lot of code that seemed like it wasn't being used that I just got rid of, I'm not sure if any of it would have been supportive of this approach or if it was just left over from some unrelated experimentation. But then that seems like it would require some pretty significant rearchitecting of not just the way variables work but the way the whole viewer backend works. I'm also not sure if moving substitutions out of the client side completely would make it harder to use variables in nav links - or since the substitution would be done already if the nav links would just have the correct text to begin with. I can look into limiting the server-side pre-baked substitutions to just everything downstream of the Assessment. I'd taken a really tentative step in this direction before but aborted because I couldn't see any obvious convenience tools for traversing a draft tree in reverse (there's a |
The solution I ended up coming up with was a bit of a hybrid. When an attempt is started, each chosen question will have all of its descendent nodes checked for variables. After all of the variables used anywhere in the question have been aggregated, for each variable all of the question's ancestor nodes will be checked until the closest one that owns a variable of the same name. Once a match is found, a reference is stored as part of the Luckily this only has to happen the one time, and then whenever So if a question contains any text nodes with a
|
… children as 'parentId' for easier traversal upwards through the tree if necessary. When starting an assessment attempt, all variables within chosen questions will have their nearest owners tracked and any shorthand variable strings will be replaced with longhand variable strings so substitution works properly in all cases. Replaced a separate but identical loop to process chosen questions when resuming attempts with the existing utility function used when starting or reviewing attempts. Added new utility functions for identifying variables within a question's node tree, determining the nearest owner of variables to a given node, and replacing variable text.
THIS PULL REQUEST HAS MIGRATIONS.
Closes #1147.
Closes #1814.
Significant progress towards #554.
Adds variable creation and substitution and experimental feature flags.
Adds a 'state' column to the 'visits' table to store the variables generated for a visit rather than regenerating them every time.
FEATURE FLAGS
This PR adds the ability to enable experimental features that are not ready for general user consumption but could potentially be used by power users. Currently this is handled at the browser level using
localStorage
, which can be set manually or simplified using a globally available API. For example, to enable the variable creation UI in the editor, run the following in the console:VARIABLES
Variables can be defined on any node or page in a module or at the top level of the module itself. Variables should be automatically available to any node in the hierarchy under the location at which the variable is defined, but can also be referred to by variables not in the vertical hierarchy by using specific notation.
For example, suppose the module has defined variables
$a
and$b
. AText
node on any page should be able to use those variables automatically:Or, for example, a text node with an ID of 'some-text-node' could define a variable
$c
. Another text node - a sibling, or a text node on another page - can also use that variable:Things that still need doing:
converter.js
required an additional line to make sure variables remain attached to the node'scontent
, and every node's corresponding JSON parser function had to be altered in order to make sure XML translation contains variables properly.seriesType
attribute for random sequence variables, since it can be given an invalid value manually in the code editors which don't translate well in the visual editor.It looks like the Question nodes generated in assessment reviews lack the 'parent' linkage that eventually leads all the way back up to the module - see the 'Problems' section below.Fixed.This works in most cases, but gets weird in assessments with nested question banks. If nested question banks are used, the question nodes are rendered separate from the question bank node that 'contains' them - see the 'Problems' section below.Fixed.It turns out the text that appears in the nav area is somehow separated completely from the nodes used to determine what links appear in the nav area, so the usual variable substitution process doesn't work at all. Absent some clever solution that does the substitution on the server side, we may have to let this ride for a bit.Passing the parent model along when generating nav items makes them available when trying to perform variable substitutions on the nav text, as those models are required when trying to locate the value generated for any given variable. That seems to be adequate.PROBLEMS
Given the settings on the primary question bank, a single one of the nested question banks will be selected, and all questions in the selected bank will be used in the assessment. If both questions are using a variable in the simplified format (e.g.
{{$qb}}
), then the value substituted in should be the value of$qb
defined on the nested question bank that is chosen, meaning if the second question bank is chosen, the value displayed would be 'qb2'. This is not what happens. Since the question node is isolated from the rest of the draft to be rendered during the assessment, its containing question bank node is not rendered and not attached to the question node as aparent
- so instead of the displayed value being 'qb2', it is 'qb0'. Likewise the substitution for$other
does not work at all, because the node it would be found attached to does not exist anywhere it can be reached by the variable utility.parent
properties and therefore the variable substitution utility can not traverse the question node's ancestors to determine the owner of the variable being used. For example, take the following structure:During an assessment attempt
$m
,$a
and$qb
will all substitute properly because the node's ancestral hierarchy is fully available via theparent
property of the question node and each subsequent parent. During review, however, the question node is rendered without any part of this hierarchy. As a result, none of the variables are substituted at all.