Skip to content
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

Only show the missing trigger variable warning for Story Table Steps #452

Closed
plocket opened this issue Dec 25, 2021 · 1 comment · Fixed by #850
Closed

Only show the missing trigger variable warning for Story Table Steps #452

plocket opened this issue Dec 25, 2021 · 1 comment · Fixed by #850
Labels
enhancement New feature or request

Comments

@plocket
Copy link
Collaborator

plocket commented Dec 25, 2021

The trigger variable only relevant for Story Table steps trying to identify unique pages for fields that use index variables or generic objects (see #256). In those cases, the developer needs to add some special lines in their interview to give us that clue. We've written a warning for whenever the developer leaves the trigger variable out of their interview, but it shows up all the time, not just when it would matter.

Ideas for when the warning of a missing trigger variable should be added to the report:

  1. Whenever there's a Story Table Step.
  2. Whenever a Story Table has any rows that use index variables or generic objects.
  3. Whenever a Story Table row uses an index variable or generic object.
  4. Whenever a Story Table row includes something in its "trigger" column.
  5. Whenever a trigger variable is present in the page. Rejected because a trigger variable would be present on every page of an interview, especially if Assembly Line is included, not just those that really need them.
  6. A combination of the above.

Regardless, it shouldn't show up for other types of Steps, even if they set variables. The page where the value is meant to be set is already clear in that situation.

I'm reluctant to pass another parameter into scope.setFields(), but that may be the only way to do it.

Edit: See docs for trigger var: https://suffolklitlab.org/docassemble-AssemblyLine-documentation/docs/automated_integrated_testing/#a-missing-trigger-variable

@plocket
Copy link
Collaborator Author

plocket commented Jan 4, 2022

We might do this by giving non-story-table Steps a fake 'trigger' variable. Something like 'ALKiln: no trigger variable needed'. That way, it won't be undefined. [We'd then check the value of the story table's trigger variable to somehow prevent page field detection from logging that error? I'm unsure. Maybe we move that error up out of page field detection into setFields itself.]

It'd still be nice to reduce this to only printing once per page. [Maybe that could be a separate issue, but taking care of it here would be acceptable.]

plocket added a commit that referenced this issue Feb 12, 2024
* Add the new proxy var HTML node

* Use a different arbitrary url that hopefully times out less often

* Remove weird whitespace characters

* Some refactoring and lang improvments prepping for double columns

* Include first varname value in the list of guesses

* Gather proxy-substituted DOM var names without breaking old code

* Fix typo and add new tests

Co-authored-by: pjg11 <45182159+pjg11@users.noreply.github.com>

* Add new method docstring

* Update fixtures for unit tests, avoid absent proxies

TODO:
- Update signature too
- Make issue: `not_decoded_name = href.replace(/^[A-Za-z0-9]*/, '_');` may need `g` flag

* Clean up with a bit of refactoring

* Test with the current version

* Add new test files and docs

* Write fixture tests, fix undefined var

* Restore simple.pdf

* Update changelog

* Reduce the trigger var warnings a la #452, add comments

* Clean up unused code

* Use less problematic suffix for name tests

* Use more robust suffix name. Improve error for missing page value

"Robust suffix name": We were setting the suffix to "Sr", but that got changed somewhere to "Sr." (note the new period). I'm not sure if that's an AssemblyLine thing or a docassemble thing. "II" shouldn't run into the same problems.

* Publish dev version with new behavior

* Identify reason for unexpected skip - wrong test status check

* Fix internal report test passes unexpectedly. Also...

- Add name of Scenario to progress dots. May help with logging progress on GitHub
- Add internal test to fail even when the report is correct (a previously undiscovered bug)
- Allow "expected status" tests to accept more than 'passed' and 'failed'
- Added doc explaining what results we expect
- Fix failures because upstream Assembly Line change (https://github.com/SuffolkLITLab/docassemble-AssemblyLine/blob/f9bd864990f8bc2c0c0096700ba6ad647772cb83/docassemble/AssemblyLine/al_general.py#L1008) added punctuation

Co-authored-by: justcho5 <justcho5@gmail.com>

* Change npm version to test GitHub issues

* Fix version typo

* Make sure failure test suffix is wrong under all circumstances

* Test PR on branch making the PR instead of the PR target branch

---------

Co-authored-by: pjg11 <45182159+pjg11@users.noreply.github.com>
Co-authored-by: justcho5 <justcho5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant