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

Enforce people entering numbers for hours #193

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Conversation

BryceStevenWilley
Copy link
Contributor

We had input type: number, which doesn't exist, and so was doing nothing. Changed to datatype: number, and added validation messages that should show if the user enters something that's not a number.

We had `input type: number`, which doesn't exist, and so was doing nothing.
Changed to `datatype: number`, and added validation messages that should show
if the user enters something that's not a number.
@BryceStevenWilley
Copy link
Contributor Author

A bit more context here: there are production errors that have the following error:

  File "/usr/share/docassemble/local3.10/lib/python3.10/site-packages/docassemble/ALToolbox/al_income.py", line 1246, in gross_total
    total += job.gross_total(
  File "/usr/share/docassemble/local3.10/lib/python3.10/site-packages/docassemble/ALToolbox/al_income.py", line 1082, in gross_total
    total += self._item_value_per_times_per_year(
  File "/usr/share/docassemble/local3.10/lib/python3.10/site-packages/docassemble/ALToolbox/al_income.py", line 1039, in _item_value_per_times_per_year
    value * Decimal(self.hours_per_period) * Decimal(frequency_to_use)
decimal.InvalidOperation: []

I hadn't been able to fix this without looking at the inputs that people give, because the error doesn't have enough information. But I discovered it was because people were entering full sentences in that field, which obviously causes a conversion error.

I looked at trying to patch al_income.py specifically, but I do think that throwing an exception is the right thing to do in that case; the class just shouldn't have full text strings for those values, and if it does at that point, I don't think there's anything we could do 1.

Footnotes

  1. We could try to delete the offending attribute and call log(..., "error") which shows text to users directly, but if we did, the user would just get sent back to the screen where they entered the information, and some text at the top. We don't know what the field name on that screen is, or anything else. If we're interested in that fix, let me know and I could make it.

@BryceStevenWilley
Copy link
Contributor Author

@nonprofittechy , @plocket Realized that I forgot to mention this fix I made last week on deep-dive today.

The thing I wanted to float was if we thought that the solution I mentioned here is a good idea; otherwise, existing interviews where people have entered non-numbers will continue to be broken.

@nonprofittechy
Copy link
Member

@nonprofittechy , @plocket Realized that I forgot to mention this fix I made last week on deep-dive today.

The thing I wanted to float was if we thought that the solution I mentioned here is a good idea; otherwise, existing interviews where people have entered non-numbers will continue to be broken.

@BryceStevenWilley that's a pretty interesting idea. I'm willing to try it. I suspect that the existing broken interviews are just abandoned forever though, so it's not necessarily urgent.

@BryceStevenWilley
Copy link
Contributor Author

BryceStevenWilley commented Sep 26, 2023

Tested and it works fine. TBH I have more concerns about the number validation UX messages than I do about sending people back, but that's a separate issue.

I suspect that the existing broken interviews are just abandoned forever though, so it's not necessarily urgent.

That's true, it's not urgent, but it is a nice safety feature to have as well IMO, in case other override the question.

@BryceStevenWilley BryceStevenWilley merged commit 7bdc847 into main Sep 26, 2023
4 checks passed
@BryceStevenWilley BryceStevenWilley deleted the conversion_syntax branch September 26, 2023 18:03
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 this pull request may close these issues.

2 participants