Close #706 improve loops

CHANGELOG.md

@@ -45,7 +45,8 @@ Format:
   be run in any directory, not just in an npm package.
 - Additional environment variables and their validation to allow for tests that run on a developer's server/Playground instead of through GitHub. Also, other functionality for that purpose. [Issue #661](https://github.com/SuffolkLITLab/ALKiln/issues/661)
 - Tests for new session_vars behavior and improve previous tests.
-- Adds `npm-shrinkwrap.json`, so installs from npm will have fixed version dependencies
+- Adds `npm-shrinkwrap.json`, so installs from npm will have fixed version dependencies.
+- Allow author to specify loops with only `.target_number`. e.g. to leave out `.there_are_any`. See [issue #706](https://github.com/SuffolkLITLab/ALKiln/issues/706) and Story Table documentation in docs folder.
 
 ### Changed
 - BREAKING: the github action no longer runs `npm run XYZ`; it directly calls scripts,
@@ -66,6 +67,8 @@ Format:
 - Updated field decoding to handle new object field encoding. See [#711](https://github.com/SuffolkLITLab/ALKiln/issues/711)
 - Allow multiple languages to be tested again. See [#713](https://github.com/SuffolkLITLab/ALKiln/issues/713).
 - Fill in time fields correctly. See [#726](https://github.com/SuffolkLITLab/ALKiln/pull/726).
+- Allow `.target_number` to be 0. See https://github.com/SuffolkLITLab/ALKiln/issues/706.
+- Use the right number of loops for `.target_number`. See https://github.com/SuffolkLITLab/ALKiln/issues/706.
 
 ### Security
 - Pass docassemble API keys through HTTP headers instead of as parameters.

docassemble/ALKilnTests/data/questions/test_loops.yml

@@ -0,0 +1,66 @@
+metadata:
+  title: Loop tests
+  description: Test loops with .there_is_another and .target_number
+---
+# Necessary to tell us what the sought var is on each page
+# Every interview that wants testing will need to have an element like this
+default screen parts:
+  post: |
+    <div data-variable="${ encode_name(str( user_info().variable )) }" id="trigger" aria-hidden="true" style="display: none;"></div>
+---
+objects:
+  - there_are_any_people: DAList.using(object_type=Individual)
+  - there_is_another_people: DAList.using(object_type=Individual, there_are_any=True)
+  - target_people: DAList.using(object_type=Individual, ask_number=True)
+---
+mandatory: True
+code: |
+  there_are_any_people.gather()
+  there_is_another_people.gather()
+  target_people.gather()
+  end
+---
+id: there are any people
+generic object: DAList
+question: Are there any "${ x.object_name() }" people?
+yesno: x.there_are_any
+---
+id: is there another person
+generic object: DAList
+question: Are there more "${ x.object_name() }" people?
+yesno: x.there_is_another
+---
+id: target number
+question: How many "target_number" people are there?
+fields:
+  - Number of other people: target_people.target_number
+    input type: number
+---
+id: person name
+generic object: DAList
+question: Name of the "${ x[ i ].object_name() }" person
+fields:
+  - Name: x[i].name.first
+---
+id: end
+event: end
+question: The end!
+subquestion: |
+  `there_are_any_people` people: ${ len(there_are_any_people.complete_elements()) }
+
+  % for person in there_are_any_people:
+  * ${ person }
+  % endfor
+
+  `there_is_another_people` people: ${ len(there_is_another_people.complete_elements()) }
+
+  % for person in there_is_another_people:
+  * ${ person }
+  % endfor
+
+  `target_people` people: ${ len(target_people.complete_elements()) }
+
+  % for person in target_people:
+  * ${ person }
+  % endfor
+---

docassemble/ALKilnTests/data/sources/reports.feature

@@ -423,6 +423,24 @@ Scenario: Warn when there are too many names
   And I set the name of "users[0]" to "Uli Udo User Sampson Jr"
   And I tap to continue
 
+@rw2 @table @loops
+Scenario: Warns about invalid `there_is_another | True` is in a table.
+  Given the final Scenario status should be "passed"
+  Given the Scenario report SHOULD include:
+  """
+  The attribute `.there_is_another` is invalid in story table tests
+  """
+  Given I start the interview at "test_loops.yml"
+  And the max seconds for each step is 5 seconds
+  And I set the var "x.there_are_any" to "True"
+  And I set the var "x[i].name.first" to "AnyPerson1"
+  And I tap to continue
+  # Should correctly get to "Name of the “first there is another people” person" question
+  And I get to "person name" with this data:
+    | var | value | trigger |
+    | x[i].name.first | AnyPerson2 | there_are_any_people[1].name.first |
+    | x.there_is_another | True | there_are_any_people.there_is_another |
+
 
 # ===============================
 # Reports for "passed" Scenarios
@@ -539,12 +557,18 @@ Scenario: Sign in to server successfully
   Given I sign in with the email "USER1_EMAIL" and the password "USER1_PASSWORD"
 
 @rp4 @table
-Scenario: Report doesn't complain about `there_is_another | False` in a table.
-  Given the Scenario report should not include:
+Scenario: Passes with no warning when `there_is_another | False` is in a table.
+  Given the final Scenario status should be "passed"
+  Given the Scenario report should NOT include:
   """
   The attribute `.there_is_another` is invalid in story table tests
   """
-  Given I start the interview at "test_gather"
-  And I get to "end screen" with this data:
+  Given I start the interview at "test_loops.yml"
+  And the max seconds for each step is 5 seconds
+  And I set the var "x.there_are_any" to "True"
+  And I set the var "x[i].name.first" to "AnyPerson1"
+  And I tap to continue
+  And I get to "person name" with this data:
     | var | value | trigger |
-    | users.there_is_another | False | users.there_is_another |
+    | x[i].name.first | AnyPerson2 | there_are_any_people[1].name.first |
+    | x.there_is_another | False | there_are_any_people.there_is_another |

docassemble/ALKilnTests/data/sources/story_tables.feature

@@ -89,3 +89,51 @@ Scenario: I upload files with a table
     | upload_files_visible | some_png_1.png, some_png_2.png |  |
     | show_upload | True |  |
     | upload_files_hidden | some_png_2.png |  |
+
+@slow @st5 @loops
+Scenario: 0 target_number for there_are_any and target_number lists, 1 for there_is_another
+  Given I start the interview at "test_loops.yml"
+  And I get to "end" with this data:
+    | var | value | trigger |
+    | x.target_number | 0 | there_are_any_people.target_number |
+    | x.target_number | 1 | there_is_another_people.target_number |
+    | x[i].name.first | AnotherPerson1 | there_is_another_people[0].name.first |
+    | target_people.target_number | 0 |  |
+  And I SHOULD see the phrase "there_are_any_people people: 0"
+  And I SHOULD see the phrase "there_is_another_people people: 1"
+  And I SHOULD see the phrase "target_people people: 0"
+
+@slow @st6 @loops
+Scenario: target_number 2 for there_are_any, there_is_another, and target_number lists
+  Given I start the interview at "test_loops.yml"
+  And I take a screenshot
+  And I get to "end" with this data:
+    | var | value | trigger |
+    | x.target_number | 2 | there_are_any_people.target_number |
+    | x[i].name.first | AnyPerson1 | there_are_any_people[0].name.first |
+    | x[i].name.first | AnyPerson2 | there_are_any_people[1].name.first |
+    | x.target_number | 2 | there_is_another_people.target_number |
+    | x[i].name.first | AnotherPerson1 | there_is_another_people[0].name.first |
+    | x[i].name.first | AnotherPerson2 | there_is_another_people[1].name.first |
+    | target_people.target_number | 2 |  |
+    | x[i].name.first | TargetPerson1 | target_people[0].name.first |
+    | x[i].name.first | TargetPerson2 | target_people[1].name.first |
+  And I SHOULD see the phrase "there_are_any_people people: 2"
+  And I SHOULD see the phrase "there_is_another_people people: 2"
+  And I SHOULD see the phrase "target_people people: 2"
+
+@slow @st7 @loops
+Scenario: target_number 1 for all people lists
+  Given I start the interview at "test_loops.yml"
+  And I take a screenshot
+  And I get to "end" with this data:
+    | var | value | trigger |
+    | x.target_number | 1 | there_are_any_people.target_number |
+    | x[i].name.first | AnyPerson1 | there_are_any_people[0].name.first |
+    | x.target_number | 1 | there_is_another_people.target_number |
+    | x[i].name.first | AnotherPerson1 | there_is_another_people[0].name.first |
+    | target_people.target_number | 1 |  |
+    | x[i].name.first | TargetPerson1 | target_people[0].name.first |
+  And I SHOULD see the phrase "there_are_any_people people: 1"
+  And I SHOULD see the phrase "there_is_another_people people: 1"
+  And I SHOULD see the phrase "target_people people: 1"

docs/story_tables.md

@@ -0,0 +1,89 @@
+# Story Tables overview
+
+The Story Table is a very flexible tool. It lets the author answer interview (form) questions in any order. They can make cosmetic changes to their interview code without having to change their tests. They can edit the order of their interview's pages, move fields from one page to another, and so on.
+
+When the robot arrives on each interview page, it finds all the fields on the page and loops through the whole Story Table trying to match rows with fields.
+
+Here's some [documentation on the use of Story Table rows](https://suffolklitlab.org/docassemble-AssemblyLine-documentation/docs/alkiln/#story-tables).
+
+## Tracking use of rows
+
+Why do we track how many times each Story Table row was used to set a form's fields?
+
+First, it lets us give the author useful information. We can show the author which rows of their story table were or weren't used. This can help the author tell that some crucial variables aren't being set as they expected. It also outputs a new Story Table for them. If they had a bunch of extra rows adding cruft, like from our [story table generator](https://plocket.github.io/al_story/), it gives the author a table with only the variables that are needed for the test.
+
+Second, in some cases we need to actually throw an error if any row was left unused, so we have to keep track of that.
+
+Third, looping questions that gather multiple items in a list are tricky when the interview answers need the `.there_is_another` attribute. We can't use that attribute. Instead, we handle all gathering with `target_number` rows. Next we'll discuss what problem the `.there_is_another` attribute creates and our approach to solving it.
+
+## The loop problem
+
+Since the Story Table loops through all its rows for each page, each row in the table needs to be unique. That is, you can only set one variable to one value. That's fine most of the time, but when gathering items in a list in a docassemble interview, there are a few ways to do it and one of them sets one variable over and over again - `.there_is_another`. This kind of loop can start with a question that sets `.there_are_any`.
+
+For example, look for `.there_is_another` in the scenario below:
+
+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`)
+Page 3: Do you have any other moms? (Set `moms.there_is_another` to `True`)
+Page 4: What is your second mom's name? (Set `mom[1].name.first`)
+Page 5: Do you have any other moms? (Set `moms.there_is_another` to `False`)
+
+See that on page 3, `moms.there_is_another` is set to `True` and on page 5 it's set to `False`. We can't represent this in a Story Table.
+
+```
+| x.there_is_another | True | moms.there_is_another |
+```
+
+The above will always say that we have another `mom`. If we allowed our users to do that, it would create an infinite loop. So we prevent our users from creating a `there_is_another`  row that has a value of `True`. We still need to allow them to set `.there_is_another`, though, so what do we do?
+
+## The loop solution
+
+Well, there's another way to loop to gather items in docassemble - `target_number`. For example:
+
+Page 1: How many moms do you have? (Set `moms.target_number` to `2`)
+Page 2: What is your first mom's name? (Set `mom[0].name.first`)
+Page 3: What is your second mom's name? (Set `mom[1].name.first`)
+
+The target number row looks like this:
+
+```
+| x.target_number | 2 | moms.target_number |
+```
+
+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: 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 (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 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 (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 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 the relevant `.used_for['foo']` by 1.
+            2. Increment all relevant `.times_used`[^2] by 1.
+    4. Try to continue.
+
+<!-- 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.
+
+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. 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.