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

Refactor: get all fields on the page then compare to story table #158

Closed
5 tasks done
plocket opened this issue Feb 28, 2021 · 12 comments
Closed
5 tasks done

Refactor: get all fields on the page then compare to story table #158

plocket opened this issue Feb 28, 2021 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@plocket
Copy link
Collaborator

plocket commented Feb 28, 2021

Edit 2 (03/23):
Turning this into a reworking of the framework. This need was highlighted by the trouble calculating variable names of proxy variables (x, i, etc).

Changes for v2:

  • Story table will have three columns that are basically | var name | value | checked |. We need another word for checked. This will make it much easier for developers to know what to put in each column. It will also make it more straightforward to check against those values. Not completely sure this part will work out.
  • Support previous format of story table. Still not sure what 'checked' should be.

Refactors for v2:

  • First use the string of the html to collect the data about the page (sought var, field selectors, field values), then check that against the table, setting values appropriately.
  • Use sought var to figure out actual var names and values for proxy vars (x, i, j...).
  • Clean up scope.js. Are there outdated functions in there? Do we need to split it into multiple files? This is holding up this issue and really belongs in its own issue.

Testing changes for v2:

  • Add unit tests based on data, like html strings. Simpler to maintain. Less conflict with different tests trying to use the same project name (though the fix for that is in an issue already).

Edit 1:
We may have to do a big refactor to account for this newly discovered situation. That may include getting rid of one column in the table. Which of the three columns in the table need what information is pretty confusing as it is anyway.


https://docassemble.org/docs/fields.html#index%20variables. Also relevant to generic objects: https://docassemble.org/docs/modifiers.html#generic%20object.

This poses a particular problem. DOM uses literally 'i', not var name. This is fine as long as the answers are as interchangeable as this implies. If the specific variables' answers matter later on, it's more of a problem. It also poses a problem for testing generated PDFs because the order in which the vars might be set is not controlled, so you have the potential of coming out with a different PDF just because of a small change. It wouldn't even make a difference if we sort the rows alphabetically or something. If the developer changes the name of the variable, it will get out of whack. What is the solution here?

A simple example: your_past_benefits['State Veterans Benefits']. Here we can just replace what's inside the brackets with an 'i'. If something gets more complicated (e.g. thing[i]['bar']), or thing['bar'][i] or thing[i][j] do we create all combos?

Trying to discuss this upstream.

@plocket
Copy link
Collaborator Author

plocket commented Mar 2, 2021

plocket/docassemble-ALAutomatedTestingTests#56 is related to this as well. [I think this was transferred to #159]

@plocket
Copy link
Collaborator Author

plocket commented Mar 2, 2021

Using /api/session is not interacting with a web page the way that a person would, but neither is your approach of trying to populate elements on the page using Python variable names. A person would interact with the screen by reading labels and clicking on things based on their labels, which you don’t want to do.
The response from POSTing to /api/session is a JSON representation of the current question (same as a GET to /api/session/question), and that JSON representation contains all of the information needed to create a UI for asking the question. That question is the same question that the user would see if they were using the web browser to go through the interview. So it is not different in substance from what the user would see during the interview. Using /api/session you can visit every question that the user would see, in the same order as the user. The advantage of the JSON representation is that it is easily machine readable. It also shows the variable names associated with the fields, so that you know what variables you should set when you pass a variables dictionary to the /api/session POST endpoint.

https://docassemble.org/docs/api.html#manage_api

For reference, I think this is using the above methodology: https://github.com/jhpyle/docassemble/blob/master/docassemble_demo/docassemble/demo/random-test.py

[I'm not sure how we would load those pages into something that puppeteer can test for other things, such as terms working properly, etc. Maybe everything would have to be overhauled to parse the data/html of the question somehow.]

@plocket
Copy link
Collaborator Author

plocket commented Mar 2, 2021

For now, we can at least handle the most basic case - foo[i].

@plocket plocket transferred this issue from SuffolkLITLab/docassemble-ALKilnSetup Mar 3, 2021
@plocket
Copy link
Collaborator Author

plocket commented Mar 3, 2021

Experiments going on here: https://repl.it/@plocket/findi#index.js

@plocket plocket added the bug Something isn't working label Mar 3, 2021
@plocket plocket changed the title Test questions created with index variables Get questions created with index variables Mar 3, 2021
plocket added a commit that referenced this issue Mar 3, 2021
Vars might be set out of order.
@plocket
Copy link
Collaborator Author

plocket commented Mar 3, 2021

All the part leading up to the 'proxy' var (x, i, j, etc.) and sought variable logic are hard-coded, but here's a version that may fit better with our current data structure: https://repl.it/@plocket/index-vars#index.js

@plocket
Copy link
Collaborator Author

plocket commented Mar 3, 2021

I need a sanity check on the assumptions, but I think this method can overcome the current obstacle to automated testing.

Summary of use:

  1. Find all unique base64 field identifiers on the page.
  2. Figure out the various index var and generic object patterns that may exist in those. For example, x[i].foo[j]'s constituent parts would be "", x, [, i, ]. foo[, j, and ]. It will be stored in various forms. For example, regex will be created that can pattern match. For now we'll call things like x, i, and j 'proxies'.
  3. See if the sought variable (proposed in this AL issue) matches any of the patterns.
  4. If so, use the pattern to get the intrinsic name values of the proxies in the pattern. For example, i might be a proxy for 'TAFDC' on this particular page.
  5. Use that mapping to find out the intrinsic names of other variables on the page.
  6. Try to match the intrinsic names with values the developer gave in the cucumber Scenario.
  7. If there's a match, set the relevant field appropriately.

Known assumptions include:

  1. It is not possible to have multiple proxy variables patterns on the page that will match the sought variable.
  2. It is not possible to have proxy variables (x, i, j, etc.) that appear on the page but are not in the sought variable.

As linked above, see example implementation of proxy var logic.

plocket added a commit that referenced this issue Mar 5, 2021
Re #158, temp fix for simple index_var detection
@plocket
Copy link
Collaborator Author

plocket commented Mar 14, 2021

A suggestion has been raised to change scope.getField() (and other functionality) into something that's unit test-able. That would mean getField would parse a string instead of interacting with the DOM. It sound like a reasonable idea to me, though it would add another layer on top of the testing. It might be easier to test more thoroughly that way, though.

If we were to do that, I think https://cheerio.js.org/ might be the way to go.

@plocket
Copy link
Collaborator Author

plocket commented Mar 17, 2021

As far as logic goes, the one confounding factor I can see for removing one column from the story table is the inability to literally set a checkbox to false. That is, to unselect a checkbox that has been selected by default. In the table, we can make a rule that to set something to false, you leave it out of the table completely. Anything absent from the table would be set to false.

It does seem like it would be confusing to users, though. So. Not sure how to handle that. And not sure how to otherwise simplify the table. Maybe the only thing in the right-hand column are 'true' or 'false' (where needed)? Not sure how to name those columns to make that clear.

I think we need some kind of simplification because the code involved in the refactor is otherwise getting a bit out of hand. Maybe that's not the true source of the problem, but that's my best guess right now.

@plocket
Copy link
Collaborator Author

plocket commented Mar 18, 2021

I think we do need 3 columns. It's a shame, but I think it's too unclear otherwise. If people want to actively unselect something, they'll probably want to have that in a table row.

Alternative I'm working with now:
| variable name | value | checked |

The third column could be left blank when not needed. Everything else would be squeezed into the first two columns. I'm not sure if it'll work or not, but I'm giving it a shot in https://github.com/plocket/docassemble-cucumber/tree/158_proxy_parse.

[This would move us into v2, of course.]

@plocket plocket self-assigned this Mar 18, 2021
plocket added a commit that referenced this issue Mar 23, 2021
#158 Start refactor for parsing proxies and other improvements
@plocket plocket changed the title Get questions created with index variables Refactor to get fields first to compare to story table Mar 23, 2021
@plocket plocket changed the title Refactor to get fields first to compare to story table Refactor and v2 to get fields first to compare to story table Mar 23, 2021
@plocket
Copy link
Collaborator Author

plocket commented Mar 23, 2021

Updated issue to explicitly address and outline the code changes (v2) needed for this functionality.

plocket added a commit that referenced this issue Mar 24, 2021
plocket added a commit that referenced this issue Mar 24, 2021
#158 add simple show if field tests (after #177)
plocket added a commit that referenced this issue Mar 24, 2021
plocket added a commit that referenced this issue Mar 24, 2021
#158 custom continue buttons that are not yesno or yesnomaybe
plocket added a commit that referenced this issue Mar 24, 2021
plocket added a commit that referenced this issue Mar 25, 2021
#158 Add an test an action button that triggers an event
@plocket plocket changed the title Refactor and v2 to get fields first to compare to story table Refactor: get all fields on the page then compare to story table Mar 26, 2021
@plocket
Copy link
Collaborator Author

plocket commented Mar 26, 2021

We can stick with v1 by supporting the previously used story table.

plocket added a commit that referenced this issue Mar 26, 2021
#158, implement new code for live tests
plocket added a commit that referenced this issue Mar 31, 2021
plocket added a commit that referenced this issue Apr 5, 2021
#158 refactor (needs publishing) Handle proxy vars, fix signatures
@plocket
Copy link
Collaborator Author

plocket commented Apr 9, 2021

The transfer has been completed and more thoroughly tested now. We should be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant