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.

Sign up for GitHub

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

Fixed bug that required alMax #245

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Fixed bug that required alMax #245

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions docassemble/ALToolbox/ThreePartsDate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,12 @@
// Non-MVP. Make an issue.
// Replace '-' in case it's an ISO date format, (our recommended format).
// JS doesn't play nicely with ISO format.
var min_date = new Date(get_$original_date(field).attr('data-alMin').replace(/-/g, '/'));
var min_attr = get_$original_date(field).attr('data-alMin');
if (!min_attr) {{
// Validation should always succeed if no minimum given
return true;
}}
var min_date = new Date(min_attr.replace(/-/g, '/'));

// Avoid usinig `params`, which could be in many different formats.
// Just get date data from the actual fields
Expand Down Expand Up @@ -538,7 +543,12 @@
}}

// TODO: Catch invalid alMax attr values for devs? Log in console? Make post MVP issue
let max_date = new Date(get_$original_date(field).attr('data-alMax').replace(/-/g, '/'));
let max_attr = get_$original_date(field).attr('data-alMax');
if (!max_attr) {{
// Validation should always succeed if no minimum given
return true;
}}
Comment on lines +547 to +550
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a birthdate field, it still needs to have a max birthdate. I can't make a full suggestion because it would include code that isn't changed, but I imagine would go something like this:

max_date = new Date();
max_attr = ...
if ( max_attr ) {
  max_date = max_attr stuff
} else {
  if ( !is_birthdate) { return true; }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's true? The demo doesn't specify a max date for the example birth date at all (and it doesn't look like there's any available in the DOM.

  - Birth date (today is valid): date_of_birth
    datatype: BirthDate
    required: True
    validation messages:
      required: This is a custom 'required' message.

If we do want to start enforcing the max attribute for birthdates, what is it supposed to do if there isn't a max attribute on the birthdate? I don't see any good examples of our error handling for developer errors in javascript.

Should it just keep the current PR's code, and have future dates be handled by the python validation, which always happens for Birth dates?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds to me like @plocket forgot that the birthdate validation was happening in Python, not JS. This seems good to merge to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the code in the DOM sets the max date for BirthDate to the current date as the default:

docassemble-ALToolbox/docassemble/ALToolbox/ThreePartsDate.py

Lines 552 to 554 in 27858f6

if ( isNaN(max_date) && is_birthdate(field)) {{
max_date = new Date();
}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks plocket. I misunderstood what you were saying, and I got a little lost in the code and didn't see the line right below. I'll fix this in a new PR since this was merged too soon.

let max_date = new Date(max_attr.replace(/-/g, '/'));
if ( isNaN(max_date) && is_birthdate(field)) {{
max_date = new Date();
}}
Expand Down