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

FFS-1809: Disable multiple clicking on send invitation button #400

Merged
merged 6 commits into from
Jan 3, 2025

Conversation

YvetteWhiteUSDS
Copy link
Contributor

@YvetteWhiteUSDS YvetteWhiteUSDS commented Dec 18, 2024

Ticket

Resolves FFS-1809.

Changes

Overview of the bug

When a user is presented with the invitation page, if the user clicks on the send button multiple times, then multiple duplicate invitations will be generated and sent to the invitee. The purpose of this fix is to make sure that only one invite is generated.

Summary of changes

Note: This work went through several iterations of approaches. This final approach is deemed most ideal as it maintains the use of the Turbo framework and allows for component reuse.

  1. app/app/views/caseworker/cbv_flow_invitations/new.html.erb - Wrap the invitation form in a turbo form so that form field errors will be rendered.
  2. app/app/helpers/application_helper.rb - Create a custom "form_with" that will assist with the proper rendering of page errors in the header area and default the builder to UswdsFormBuilder.
  3. app/app/controllers/caseworker/cbv_flow_invitations_controller.rb - Fixed a couple bugs that appeared when page errors (i.e., validation errors) were present. First, used flash.now instead of flash when setting the page's errors. The former will make sure the messages are shown on the page when no redirect occurs. Second, the page returns a 4XX status as opposed to a 2XX status. This fixes the error rendering at the top of the page.

Acceptance testing

  • No acceptance testing needed
    • This change will not affect the user experience (bugfix, dependency updates, etc.)
  • Acceptance testing prior to merge
    • This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
    • Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
  • Acceptance testing after merge
    • This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
    • Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

Screenshots

Validation errors are rendering properly

Screenshot 2024-12-23 at 7 08 13 PM

Screenshot of disabled button

Screenshot 2024-12-18 at 11 12 58 AM

Query of before and after an invitation was generated after the fix

Created an invitation for "Jimmy J". The first query is prior to invitation creation and the second is after.

Screenshot 2024-12-18 at 10 25 22 AM

For context, prior to changes, I was able to generate multiple invitations by continuously clicking the send button

Screenshot 2024-12-18 at 11 15 04 AM

@YvetteWhiteUSDS YvetteWhiteUSDS requested review from tdooner and GeorgeCodes19 and removed request for tdooner December 18, 2024 16:27
@YvetteWhiteUSDS YvetteWhiteUSDS marked this pull request as ready for review December 24, 2024 00:12
Comment on lines +3 to +4
# keep the database connection alive
ActiveRecord::Base.connection.execute("SELECT 1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting. What's this for?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an unrelated change that's already on main. George and I diagnosed the slow performance as lengthy database reconnections: #401

Comment on lines +36 to +37
flash.now[:alert_heading] = error_header
flash.now[:alert] = error_messages.html_safe
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, point on #now https://guides.rubyonrails.org/action_controller_overview.html#the-flash

Wasn't aware! Was the flash not appearing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was the flash not appearing?

The opposite, actually, the flash was appearing on this page and whatever next page the user went to.

The form has weird behavior currently: it POSTs to /sandbox/invitations, which adds the validation errors via flash, but then doesn't take the user to a subsequent page (rather, it renders :new view). So it leaves the user on a page they can't refresh (/sandbox/invitations) with the flash in the cookie for the next request, e.g. if they hit the back button/refresh.

So by using flash.now it makes the flash error available to the current view but not afterwards.

Comment on lines +82 to +84
turbo_frame_tag(model) do
form_with(model: model, scope: scope, url: url, format: format, **options, &block)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonderful, so this lets use keep the turbo feature without breaking forms?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, Turbo with target="_top" seems to work pretty well in general, so we could probably use it on all the other pages.

@@ -449,8 +449,6 @@ GEM
connection_pool (>= 2.2.5, < 3)
rack (~> 2.0)
redis (>= 4.5.0, < 5)
skylight (6.0.4)
activesupport (>= 5.2.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why these changes are duplicated on your branch (I think it's because it's branched off an old version of main?)

Comment on lines +3 to +4
# keep the database connection alive
ActiveRecord::Base.connection.execute("SELECT 1")
Copy link
Contributor

Choose a reason for hiding this comment

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

It's an unrelated change that's already on main. George and I diagnosed the slow performance as lengthy database reconnections: #401

Comment on lines +36 to +37
flash.now[:alert_heading] = error_header
flash.now[:alert] = error_messages.html_safe
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the flash not appearing?

The opposite, actually, the flash was appearing on this page and whatever next page the user went to.

The form has weird behavior currently: it POSTs to /sandbox/invitations, which adds the validation errors via flash, but then doesn't take the user to a subsequent page (rather, it renders :new view). So it leaves the user on a page they can't refresh (/sandbox/invitations) with the flash in the cookie for the next request, e.g. if they hit the back button/refresh.

So by using flash.now it makes the flash error available to the current view but not afterwards.

Comment on lines +82 to +84
turbo_frame_tag(model) do
form_with(model: model, scope: scope, url: url, format: format, **options, &block)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, Turbo with target="_top" seems to work pretty well in general, so we could probably use it on all the other pages.

@YvetteWhiteUSDS YvetteWhiteUSDS merged commit 014dd2d into main Jan 3, 2025
21 checks passed
@YvetteWhiteUSDS YvetteWhiteUSDS deleted the yvette/1809-multi-invites branch January 3, 2025 22:32
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