Skip to content

Commit

Permalink
From review, move some docs to comments, rearrange docs
Browse files Browse the repository at this point in the history
  • Loading branch information
@plocket
plocket committed Jul 14, 2023
1 parent 98c5768 commit b27c2df
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 145 deletions.
126 changes: 15 additions & 111 deletions docs/story_tables.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,136 +50,40 @@ The target number row looks like this:
| x.target_number | 2 | moms.target_number |
```

That's no problem at all for our code. If we require our users to use `.target_number`[^1] we have one unique variable to tell us how many times to say "Yes" to a `.there_is_another` question, letting us know when to answer "No" and stop the loop. We just also have to do a bit of extra work to make that happen.
If authors use `.target_number`, that's no problem at all for our code. We can't require authors to use that method to ask their questions, though. That may make their form questions confusing to their end users sometimes. Fortunately, we _can_ require developers to use `.target_number`[^1] in their Story Tables. We can use a `.target_number` table value to calculate what to answer for `.there_are_any` and to know how many times to set `.there_is_another` to `'True'`.

## The loop architecture

First, the very basic flow for setting a variable. Note: every variable that is set uses the same code that handles Story Table rows.
First, the very basic flow for setting a variable. Note: whatever Step the dev uses to set a variable's value, we send it to this loop. It may be useful to follow along in the code here.

1. If this isn't a Story Table step, transform the given arguments into a Story Table structure - a list of row objects.
2. For each row object
2. For each row object (see `scope.normalizeTable()`)
1. Create a new object with almost the same properties. It contains the original object in a property called `original` so the original can be used to print stuff for the user's report.
2. In this new object substitute environment variables the author gave into actual values. They may do this to keep a value (like a password) secret.
2. In this new object, substitute environment variables the author gave into actual values. They may have done this to keep a sensitive value (like a password) secret.
3. Add ways to accumulate the number of times the row is used.
3. For each page
3. For each page (see `scope.setFields()`)
1. Identify and store all the fields on the page and all the variables they set.
2. Shallowly clone the Story Table row list.
1. Create a new object almost the same as the old object
2. Print a warning to the user if they've created a `.there_is_another` row with a value of `'True'` and change that value to `'False'`.
3. If there is a `.target_number` row, use it to create new rows for `.there_is_another` and `.there_are_any`
3. If there is a `.target_number` row, use it to create new artificial rows for `.there_is_another` and `.there_are_any`.
4. This new list will be used to actually put answers into fields.
3. For each field on the page
1. Find which Story Table row, if any, matches that field.
2. Set the field to the given value.
3. Increment the row's accumulators.
1. Increment all relevant `.times_used` by 1.
2. Increment the relevant `.used_for['foo']` by 1.
1. Increment the relevant `.used_for['foo']` by 1.
2. Increment all relevant `.times_used`[^2] by 1.
4. Try to continue.

Now some details.

A row object can have two different structures (sorry). The first is for non-artificial rows:

```js
{
original: {
trigger: `moms.target_number`,
var: `x.target_number`,
value: `SECRET_NUMBER_OF_MOMS`,
},
trigger: `moms.target_number`,
var: `x.target_number`,
// These are different
value: `2`,
is_artificial: false,
times_used: 0,
used_for: { `x.target_number`: 0 },
}
```

The second is for artificial rows. An example of a `.there_are_any` row:

```js
{
source: {
// The contents of the previous outer object, with one change:
used_for: {
`x.target_number`: 0,
`x.there_are_any`: 0,
`x.there_is_another`: 0,
}
}
is_artificial: true,
var: `x.there_are_any`,
value: `True`,
trigger: `moms.there_are_any`,
used_for: { `x.there_are_any`: 0 }
// `.times_used` is the same
}
```

An example of a `.there_is_another` row:

```js
{
source: {
// The contents of the previous outer object, with one change:
used_for: {
`x.target_number`: 0,
`x.there_are_any`: 0,
`x.there_is_another`: 0,
}
}
is_artificial: true,
var: `x.there_is_another`,
value: `True`,
trigger: `moms.there_is_another`,
used_for: { `x.there_is_another`: 0 }
//`.times_used` is the same
}
```

Why is it all so nested? That's just how it evolved. It could probably use some refactoring.

The parts relevant to loops for gathering multiple items in a list are the `.target_number` accumulators and the artificial rows. Whenever a `.there_is_another` row is used to set an answer to a field, we add 1 to its `source.times_used`. That is, we mutate the `.target_number` object's `.times_used` property. We alsoadd 1 to its `source.used_for['x.there_is_another']` value. That mutates the `'x.there_is_another'` property of the `.used_for` of the `.target_number` object. We do the same for `.there_is_another` rows, but that doesn't matter for now.

Why do we increment both the `.times_used` property and the `.used_for` properties? Have patience and read on.

We set the value of a `.there_are_any` row using its `source` row's `value`. If it's greater than `0`, the value is `'True'`, otherwise it's `'False'`.

When this comes from a `.target_number` row (which I think is currently the only way we get to this stage), we set the value of a `.there_is_another` row using the `.there_is_another` accumulator in the `source` row. Three scenarios are possible.

1. If the `.source.value` is `0` and its `.used_for['x.there_is_another']` is `0`, set the `.there_is_another` `value` to `'False'`.
3. This also sounds weird, but if the `.source.value` is `> 0` and the `.source.used_for['x.there_is_another'] >= (.source.value - 1)`, return `False`. For example, if `.source.value` is `2` and `.source.used_for['x.there_is_another']` is `1`, return `False`. (We will explain this further down.)
2. Otherwise, return `True`.

Why `value - 1`? That's a wierd calculation. But think about the questions the user is being asked again:

Page 1: Do you have any moms? (Set `moms.there_are_any` to `True`)
Page 2: What is your first mom's name? (Set `mom[0].name.first`)

We just set our 1st entry in the loop and `.there_is_another` wasn't used at all.

Page 3: Do you have any other moms? (Set `moms.there_is_another` to `True` because `.source.used_for['x.there_is_another'] == 0`)
Page 4: What is your second mom's name? (Set `mom[1].name.first`)

`.there_is_another` is being used for the 1st time, even though this is actually the 2nd item in the list.

Page 5: Do you have any other moms? (Set `moms.there_is_another` to `False` because `.source.used_for['x.there_is_another'] == 1`)
This is the 2nd time `.there_is_another` is being used, but this would cause a 3rd loop if we didn't use the weird math.
<!-- From feedback, I'm not sure how to clarify this part or if readers will need it
Why make artificial rows? Well, that way when we loop through to match a page's fields to the variables in the Story Table structure, we don't have to do anything special for these rows. If they're not needed, they won't be triggered.
We did consider other options, though.

One option: have the test author set their `.target_number` value to 1 less than the number of items they want. This follows the tradition of 0 indexed languages. Unfortunately (or fortunately), the authors are generally human, not computers, and that doesn't really make sense. Also, the pattern we chose is the pattern docassemble uses for `.target_number` already.

Another option: we could just use `.times_used`. That gets incremented with `.there_are_any` as well as with `.there_is_another` and we don't need to do the weird math. The problem with this option is that sometimes the `.there_are_any` question is never asked. The author of the interview can set that ahead of time so that the user doesn't have to bother with it. In a form that's definitely about children, the author may assume the user already has children. They may exclude, "Do you have any children?" They would just ask for the first child's name right away. `.times_used` would then be off by one.

That's why we chose the weird math instead. That leaves us with a couple more questions.

`.times_used`. Why have `.times_used` at all? That's a stylistic choice. It makes later code less complicated. When we print the test report at the end and want to show the author which `.target_number` rows were actually used during the test, we need to know the total number of times the `.target_number` was used. We could add up the `used_for` values, but for now we're avoiding doing that extra math and the etra refactoring. The choice has its pros and cons.

Also, why make artificial rows? Well, that way when we loop through to match a page's fields to the variables in the Story Table structure, we don't have to do anything special for these rows. Why not calculate the `.there_are_any` and `.there_is_another` values in the row-matching loop? That's another stylistic choice. There's more logic to the `.there_is_another` value than we've talked about here and that method seems like it would add a lot more cruft and complexity in that match-finding loop. We may eventually change our minds.
Why not calculate the `.there_are_any` and `.there_is_another` values in the row-matching loop? That's another stylistic choice. There's more logic to the `.there_is_another` value than we've talked about here and that method seems like it would add a lot more cruft and complexity in that match-finding loop. We may eventually change our minds.
-->

---

[^1] Why use `target_number`? We count on our users being at least a bit familiar with docassemble variables and to have access to the docassemble docs, so the `target_number` variable is something they should be able to recognize or easily find information about. We decided to take advantage of that.
[^1] Why use `target_number`? We count on our users being at least a bit familiar with docassemble variables and to have access to the docassemble docs, so the `target_number` variable is something they should be able to recognize or easily find information about. We decided to take advantage of that. Also, we chose to avoid 0 indexing so that the number starts at 1 and feels like human language, not computer language as our users are generally more human than computer. It's also the way docassemble does it.

[^2] It would seem like the `.times_used` property is redundant, but it has a different purpose that the `.used_for` properties. It lets us show the dev whether the `target_number` table row was used at all during the test to let us give appropriate feedback to the user. We could add up the `used_for` values, but for now we're avoiding doing that extra math and the extra refactoring. The choice has its pros and cons.
92 changes: 58 additions & 34 deletions lib/scope.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,8 @@ module.exports = {
var: row.var || 'al_no_var_name',
value: actual_var_value,
trigger: row.trigger || '',
// This will track how many times total the row or its connected
// artificial rows are used. Artificial rows are only added for some rows.
times_used: 0,
used_for: {},
}
Expand Down Expand Up @@ -2137,29 +2139,37 @@ module.exports = {
/** Accumulate number of times this row was used. Esp important for `.target_number`.
* Mutates `row` and its props. */

// Increment the number of times a row has been used in total.
// Increment the number of times a row itself was used.
row.times_used += 1;

// Increment the times a row was used for a particular variable.
// TODO: check if we need this initial default assignment.
if ( !row.used_for ) { row.used_for = {}; }
// TODO: check if we need this initial default assignment.
if ( row.used_for[ row.var ] ) {
row.used_for[ row.var ] += 1;
} else {
row.used_for[ row.var ] = 1;
}

// If this is an artificial row, increment its source row as well. This may
// be one of multiple artificial rows and the source row will need to be
// incremented for each of its artificial rows.
// Right now, this is only used for `target_number` rows.
if ( row.source ) {
// Increment the number of times the source row's artificial row trackers
// were used. For example, trackers like those for `.there_are_any` and
// `.there_is_another` (for `.target_number` rows)
// For each artificial row, increment the total number of times its
// source row was used.
row.source.times_used += 1;

// Indicate which of the artificial rows was incremented (this one).
// TODO: check if we need this initial default assignment.
if ( row.source.used_for[ row.var ] ) {
row.source.used_for[ row.var ] += 1;
} else {
row.source.used_for[ row.var ] = 1;
}
row.source.times_used += 1;

// These are important for rows like `target_number` rows that are
// accumulators for the artificial rows they support.
}
} // ends if there's a source row

}, // Ends scope.increment_row_use()


Expand All @@ -2178,47 +2188,59 @@ module.exports = {

if ( from_story_table && var_datum.var.match( /\.target_number$/ )) {

// This is a row created by us, not by the user
let artificial = true;

// Add `.there_are_any` value depending on `.target_number` value to
// help allow using only `.target_number` for loop answers. If there's
// already a `.there_are_any`, that will come first and will be overriden
// by the later artificial row.
// TODO: We may need to document this as it might confuse folks who accidentally
// include both and have conflicting values for each of them.
// Add `.there_are_any` value depending on `.target_number` value.
// If the dev has also created a `.there_are_any` row, that will come
// first in the table and will be overriden by this later artificial row.
// TODO: Document this for devs as this behavior might confuse folks
// who accidentally include both and have conflicting values
// for each of them.
let var_name_any = var_datum.var.replace( /\.target_number$/, `.there_are_any` );

let there_are_any = {
artificial,
artificial,
// For now this is very nested. We may flatten in the future.
...var_datum,
// `source` is acting as the accumulator of the row use here
source: var_datum,
var: var_name_any,
// devs are supposed to use the right `trigger` name, but they might not.
// Of course, this too might be the wrong trigger name, but there's no way to
// know that and it's unlikely.
// TODO: Make sure this is documented.
trigger: var_datum.trigger.replace( /\.target_number$/, `.there_are_any` ),
used_for: {},
};
there_are_any.used_for[ var_name_any ] = 0;

// This creation function is run at the beginning of every page.
// If this is the first page and the `target_number` source row
// has just been created, it won't have this sub-accumulator yet,
// so create them. Otherwise, don't override its current value.
if ( var_datum.used_for[ var_name_any ] === undefined ) {
var_datum.used_for[ var_name_any ] = 0;
}

// If `target_number` is 0, there are none of this item
if ( parseInt(var_datum.value) <= 0 ) {
there_are_any.value = `False`;
// Otherwise, there's at least one item
} else if ( parseInt(var_datum.value) > 0 ) {
there_are_any.value = `True`;
}

// Now both `.target_number` and `.there_are_any` rows exist if needed
enhanced_var_data.push( there_are_any );

// Add `.there_is_another` value depending on `.target_number` value to
// help allow using only `.target_number` for loop answers. If there's
// already a `.there_are_any`, that will come first and will be overriden
// by the later artificial row, but the dev will still get a warning at
// the start of each page that they've included `.there_is_another` with
// a value of `True`.
// Might not be needed if `.target_number` is 0, but for now that seems
// like it would add more logic to read.
// Add `.there_is_another` value depending on `.target_number` value.
// If the dev has also created a `.there_is_another` row, that will come
// first in the table and will be overriden by this later artificial row.
// The dev will still get a warning at the start of each page that they've
// included an invalid `.there_is_another` with a value of `True`.
// This row might not be needed if `.target_number` is 0, but for now that
// seems like it would add more logic to read.
let var_name_another = var_datum.var.replace( /\.target_number$/, `.there_is_another` );

let there_is_another = {
Expand All @@ -2235,11 +2257,11 @@ module.exports = {
};
there_are_any.used_for[ var_name_another ] = 0;

// If the `target_number` has just been created, it won't have these
// This creation function is run at the beginning of every
// page. If this is the first page and the `target_number`
// source row has just been created, it won't have these
// sub-accumulators yet, so create them.
if ( var_datum.used_for[ var_name_any ] === undefined ) {
var_datum.used_for[ var_name_any ] = 0;
}
// Otherwise, don't override their current values.
if ( var_datum.used_for[ var_name_another ] === undefined ) {
var_datum.used_for[ var_name_another ] = 0;
}
Expand Down Expand Up @@ -2308,13 +2330,15 @@ module.exports = {
await scope.addToReport( scope, { type: `warning`, value: `${ val_to_print } value is not a valid value for ${ row.var } here. This test will default to \`False\` to avoid problems.` });
return `False`;

// Story table stuff
// target_number: 3 (3 items. press 'yes' on there_is_another 2 times.)
// used_for[ <var_name>.target_number ]: 0 -> `True`, 1 -> `True`, 2 -> `False`

// Check that the number of elements in the list hits our desired number.
// We have to use `+ 1` to represent that the first loop is handled
// by the value of the `.there_are_any` interview attribute instead.
// The first time `.there_is_another` is used, it's always the second item
// the form is asking about (and so forth), so we have to add 1 to this check.

// Example behavior:
// For 3 items, press 'yes' on there_is_another 2 times because
// `.there_are_any` handles the first time.
// target_number: 3
// used_for[ <var_name>.target_number ]: 0 -> `True`, 1 -> `True`, 2 -> `False`
} else if ( row.source && row.source.used_for[ row.var ] + 1 >= desired_num) { // Story table loop will end
return `False`;
} else {
Expand Down

0 comments on commit b27c2df

Please sign in to comment.