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

Fixes flash banners persisting for too long #3198

Merged

Conversation

americodls
Copy link
Contributor

Fixes #3149

@americodls
Copy link
Contributor Author

americodls commented Aug 27, 2022

Even though the issue has been created for another workflow, but this problem happens where the verify step is invoked. I've chosen this test file because the context was easier to it (I guess).

Happy to change anything if is needed.

I also have found other parts of the application where the same problem would occur.
I would be happy to fix all of them in other PR applying the test guidance I get in this PR.

@simi
Copy link
Member

simi commented Aug 27, 2022

@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #3198 (bb4d485) into master (dee7118) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3198   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files         110      110           
  Lines        3310     3310           
=======================================
  Hits         3250     3250           
  Misses         60       60           
Impacted Files Coverage Δ
app/controllers/sessions_controller.rb 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@americodls americodls force-pushed the fixes-flash-banners-persisting-for-too-long branch from 23d3830 to bb4d485 Compare August 28, 2022 00:51
@americodls
Copy link
Contributor Author

@americodls can you please fix Rubocop violation at https://github.com/rubygems/rubygems.org/runs/8053686977?check_suite_focus=true?

Done!

@simi
Copy link
Member

simi commented Aug 28, 2022

Thanks @americodls. Feel free to submit PR fixing more cases. If I understand it well (per https://guides.rubyonrails.org/action_controller_overview.html#flash-now), flash.now should be used if followed by render. Sadly there is no rubocop-rails rule for this.

@americodls
Copy link
Contributor Author

@simi I am gonna try to create a custom cop to enforce flash.now before render.
That will help me to find and fix all the other occurrences on this project.
Thanks for the feedback here!

@simi
Copy link
Member

simi commented Aug 28, 2022

@americodls that was just an idea, I have no idea if that's actually good idea. :)

@simi simi merged commit f4e4bf7 into rubygems:master Aug 28, 2022
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging August 28, 2022 13:33 Inactive
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to production September 13, 2022 15:22 Inactive
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.

Flash banners persist too long
2 participants