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

Remove the need for x in the variable name column #582

Closed
BryceStevenWilley opened this issue Jul 13, 2022 · 6 comments · Fixed by #850
Closed

Remove the need for x in the variable name column #582

BryceStevenWilley opened this issue Jul 13, 2022 · 6 comments · Fixed by #850
Assignees

Comments

@BryceStevenWilley
Copy link
Collaborator

Realized today that we can simplify the story table a lot by providing us with more information from docassemble.

We need to trigger value because generic objects don't show all of their information when being sought, just the x.sought_attribute. However, if we are on a generic object screen, we can just directly include the x.instanceName in the DOM, the same way we include the user_info().variable:

    <div data-variable="${ encode_name(str( x.instanceName if defined('x') else '' )) }" 
        id="al_generic_object" aria-hidden="true" style="display: none;"></div>
    <div data-variable="${ encode_name(str( i if defined('i') else '' )) }" 
        id="al_generic_index" aria-hidden="true" style="display: none;"></div>

This gives us the exact name of the x object (x.instanceName must be set in order for DA to seek it). This means that Kiln can take this value and substitute it with the x.sought_attribute on the page, giving the correct value of my_var.sought_attribute.

Much of the same above goes for i and j as well.

The only potential historical issue that relates to this that I can find is #158 (comment), but I don't think that affects this?

Need to think through all of the combinations of things as well, but this does seem promising.

@plocket
Copy link
Collaborator

plocket commented Jul 16, 2022

I agree this sound promising! The picture is complex enough that I'm not completely sure of the possible pitfalls. It seems like a solution that could really simplify the story table and take us into v5. We'd remove the need for the trigger variable completely.

Just fyi, we could contain all that data in one div, just using different data var names. E.g:

<div id="alkiln_proxy_var_names"
    data-generic_object="${ encode_name(str( x.instanceName if defined('x') else '' )) }" 
    data-index_var_i="${ encode_name(str( i if defined('i') else '' )) }"
    aria-hidden="true" style="display: none;"></div>

The following is true of our code even now, but may be especially relevant to this issue: I think to be fully effective this may need the loops worked out for fully digging into var names (#574) and, possibly, also need or be aided by #573 - avoid too much decoding.

@plocket plocket added this to the v5 milestone Aug 10, 2022
@plocket plocket mentioned this issue Aug 10, 2022
20 tasks
@plocket
Copy link
Collaborator

plocket commented Nov 21, 2022

Instead of breaking into v5, we could just add a new Step for this two-columns story table. As @BryceStevenWilley pointed out, we'll probably need to do some abstraction to avoid a bunch of fragile duplicate code.

@plocket
Copy link
Collaborator

plocket commented Jan 2, 2024

[Answer: I believe it goes up to 'n']

Note for (near?) future self: we need all the index vars. Do they go beyond 'm'? Look in the da core code. We can be explicit or use a loop similar to the below:

<div id="alkiln_proxy_var_values"
  data-generic_object="${ encode_name(str( x.instanceName if defined('x') else '' )) }" 
  % for letter in [ 'i', 'j', 'k', 'l', 'm', 'n' ]:
  data-index_var_${ letter }="${ encode_name(str( value( letter ) if defined( letter ) else '' )) }"
  % endfor
  aria-hidden="true" style="display: none;"></div>

[Actually, going to take myself off this for now. This will take a while and I'm working on some other things.]

@plocket plocket removed their assignment Jan 2, 2024
@plocket plocket self-assigned this Jan 31, 2024
@plocket
Copy link
Collaborator

plocket commented Jan 31, 2024

Below, I assume that users can and will do anything

x

Test for detecting and replacing x: /(^x)(?=\.|\[)|(^x$)/ /^(x)\b/.

This may not be enough. We need to test if someone can do something like set both of these on the same page:

fields:
  - Age: x.age
  - Is one of the best: best_people[x.name.first]
    datatype: yesno

Is that possible? It would be an unfortunate use case. We'll have to try it out.

Update to the above concern, these are all things I tried that caused errors:

  - Is one of the best: best_people[x.name.first]
  - Is one of the best: best_people[${ x.name.first }]
  - Is one of the best: best_people['${ x.name.first }']

i etc

Test for detecting and replacing i (global flag): /(?<=\[)([ijklmn])(?=\])/g. [Or /\[([ijklmn])\]/g if we don't mind having multiple matches along with our capture groups. Probably depends what methods we use.]

Does it need to be global? I'm thinking of a situation where we're setting a var like foo[i].bar[i]. In most situations probably not good practice, but I believe it's possible.

Procedure ideas for two-column solution

We need to keep our same decoding and list of guesses, but as we decode we can check for proxy vars. If we find them, we replace the proxy vars with the actual vars (that the user puts in the column labeled var). Also, if we do find a proxy var we can replace, we can get rid of previous guesses. After we've replaced all proxy vars, we can stop guessing.

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>
@plocket
Copy link
Collaborator

plocket commented Feb 12, 2024

We ended up not needing a new Step at all because we were already handling 2-column tables to have backwards compatibility with our first couple versions of the Story Table. These new changes shouldn't interfere with those, but

  1. We're not supporting those versions of the Story anymore
  2. I believe no one is actively using those versions of the Story anymore.

An all around win, I hope

@BryceStevenWilley
Copy link
Collaborator Author

I missed that this was closed, but I'm super happy you got to this! Thanks a ton, you did a ton of great work to push this to the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants