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

fix: CE-1150-multiple-action-taken-saved-to-the-same-complaint #768

Conversation

dk-bcps
Copy link
Contributor

@dk-bcps dk-bcps commented Nov 22, 2024

Description

This PR fixes an issue when the app does not handle well concurrent changes in complaint actions

Fixes # (CE-1150)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Open a CEEB complaint that does not have a decision section populated for edit in two browser tabs. Enter decision data and click save. Enter different decision data (select different action) in the second tab. When you click save on the second tab the app displays an error message and recommends to refresh the page.
    In the complaint list try to filter by the action you entered in the first tab. The app should find your complaint.
    In the complaint list try to filter by the action you entered in the second tab (that gave you an error). Your complain should not appear in the list.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Further comments


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:

Copy link
Collaborator

@afwilcox afwilcox left a comment

Choose a reason for hiding this comment

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

I discussed this change with @nayr974 and we both agree that we think that this validation should be moved out of the reducer and into the case management backend

I'm guessing that the case management backend is probably doing an upsert and you just need to make sure that the action doesn't exist before inserting it (and then you can return a message if it did)

@nayr974
Copy link
Collaborator

nayr974 commented Nov 25, 2024

I discussed this change with @nayr974 and we both agree that we think that this validation should be moved out of the reducer and into the case management backend

I'm guessing that the case management backend is probably doing an upsert and you just need to make sure that the action doesn't exist before inserting it (and then you can return a message if it did)

To elaborate a bit, having the validation in the back-end:

  • Prevents an extra round trip between the front end back end
  • Significantly reduces the chance of a timing issue, if somebody is on a cell connection and it takes many seconds to fetch the record, validate, and send the update another user could commit a change during that window that avoids the validation
  • Removes business logic from the Redux reducers. I don't think it's a good pattern to put validation and logic in the reducers, for code maintainability they should be as simple as possible and having validation here is outside of their intended purpose

Let us know if you want to chat or make a case against moving it to the back-end :)

Copy link

@afwilcox afwilcox merged commit d77bae4 into release/yellow-boring-sponge Nov 26, 2024
15 checks passed
@afwilcox afwilcox deleted the bug/CE-1150-Multiple-Action-taken-saved-to-the-same-complaint branch November 26, 2024 20:00
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.

3 participants