rubyforgood / casa

Volunteer management system for nonprofit CASA, which serves foster youth in counties across America.
https://casavolunteertracking.org/
MIT License
305 stars 472 forks source link

case contact creation date bug #5860

Open elasticspoon opened 3 months ago

elasticspoon commented 3 months ago

Volunteers report having intermitent bugs with dates when creating case contacts. Sometimes the date they enters into a new contact is saved as the current date. Sometimes they will even edit the contact and the updated date will not save till a third retry.

This is an intermittent failure that we have been unable to reproduce. The timing loosely aligns with this PR https://github.com/rubyforgood/casa/pull/5609 being merged so it may be a culprit.

thejonroberts commented 1 month ago

I would think that using browser native date input would be more consistent, this is a tough one.

I looked at this a bit. Didn't find anything that would obviously lead to the bug -- but I have some thoughts.

  1. CaseContact model uses a strange enum implementation for string column status. Rails may get confused by this (and devs certainly could be -- I was). This enum setup is how the status active?, started?, details?, etc.. methods used by the wicked wizard form are setup, and don't seem to do anything else. This is probably fine.
  2. CaseContact occurred_at has three validations, that run only if status is active_or_details? (determined by above).
    • We can change them to run anytime record is not started, because after first form details page, where date selection happens, contact should never go back to 'started' status. This may help validations run when expected, especially during edits -- I'm unsure what status should be during an edit step, do we kick things back from active to the edit form step? What if that edit is abandoned? We don't update status once status == 'active'

I don't think this will fix the issue. But it may give better validation feedback to users? I have done some work already. Can look at the work here.

We could also log attributes when contacts are update? Get some actual idea of what date/format is sent...

I think that in order to get some idea of what is going on, we should probably know what browsers (and versions, if possible) users are having this trouble on.

thejonroberts commented 1 month ago

Can ignore my previous comment.

One idea I have is to change the date input from the RangedDatePicker component to a straight rails form.date_field, and let standard browser & rails validations happen. RangedDatePicker does not set standard max_date/min_date for the input but sets them in data attributes, and handles the range validation via js (in app/javascript/src/validated_form.js) to do this:

Image

(note also 'minimize' does nothing on it currently (in safari at least), and will cover fields / submit button)

It would also be more consistent with validation for rest of the form. Is changing that an option?

The only other idea I have is to make a tagged log on every occurred_at change with relevant info.

thejonroberts commented 1 month ago

The javascript from app/javascript/src/case_contact.js also runs every time the form (or any page?) loads and changes the value of the input, in a kind of hacky way that browser input may not like?

function enGBDateString (date) {
  return date.toLocaleDateString('en-GB').split('/').reverse().join('-')
}
thejonroberts commented 1 month ago

Taking out that date conversion on page load doesn't break any other specs, so I'll work up a PR for that. I can't guarantee it's the issue, but seems very likely to me!

elasticspoon commented 1 month ago

interesting. A thing that I was thinking of and I am not sure if it is even logged as an issue currently is there is a page /all_casa_admins/patch_notes. And on that page if you set a patch note often times after you save the page and reload some of the drop down inputs will change.

I could not replicate what the volunteer was seeing which is why I don't have much to add here but that error is sorta similar so maybe there is a common thread?

thejonroberts commented 1 month ago

hmm... looks like a similar setup in app/javascript/src/all_casa_admin/patch_notes.js, which uses same notification library, and does some heavy DOM manipulation, but separate from the javascript involved here.

thejonroberts commented 1 month ago

Opened a PR (link above). I said that it "addressed" this issue rather than "resolves" since we don't know the precise issue. It will not auto-close this issue, we should probably confirm with users whether or not it has improved?

bcastillo32 commented 1 week ago

@elasticspoon @thejonroberts Users are encountering bugs when attempting to use the current day for case contacts, receiving an error stating that the date cannot be in the future. Could these issues be related?

thejonroberts commented 1 week ago

Could be related, but I'm not sure. I thought that we'd be safe from that situation, but appartantly not... we had some discussion of it in #5974 .

We could just push the backend validation back a day to give some time zone "cushion" until we figure out what the actual issue is?

bcastillo32 commented 1 week ago

Could be related, but I'm not sure. I thought that we'd be safe from that situation, but appartantly not... we had some discussion of it in #5974 .

We could just push the backend validation back a day to give some time zone "cushion" until we figure out what the actual issue is?

@thejonroberts Thanks for the reply - I will of course defer to you and @elasticspoon. a few users have reported this issue and was also shared on our last stakeholder meeting.

bcastillo32 commented 1 week ago

Could be related, but I'm not sure. I thought that we'd be safe from that situation, but appartantly not... we had some discussion of it in #5974 . We could just push the backend validation back a day to give some time zone "cushion" until we figure out what the actual issue is?

@thejonroberts Thanks for the reply - I will of course defer to you and @elasticspoon. a few users have reported this issue and was also shared on our last stakeholder meeting.

@thejonroberts i can create a new bug ticket. Would you like to pick it up? :)

thejonroberts commented 1 week ago

@bcastillo32 can do!

thejonroberts commented 6 days ago

I put a PR up that just adds an extra day cushion for validation. Potential that contacts will be saved as occurring 'tomorrow' instead of seeing the error.

@bcastillo32 do you have any idea if the users are seeing the issue at a certain time of day? I think we could add some specs similar to the approach here to figure out what time of the day this happening & test against it.

bcastillo32 commented 5 days ago

@thejonroberts maybe hold off on the PR - users are reporting it is happening intermittently. I can ask them to keep track of when during the day it happens to see if we can see if that is making a difference. Can we hold off on that cushion for now?

elasticspoon commented 5 days ago

Too late, I have added it. That said I don't think 1 additional day of cushion could have a negative impact

bcastillo32 commented 5 days ago

Too late, I have added it. That said I don't think 1 additional day of cushion could have a negative impact

Ok thank you both!