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

Improve banner expiration validations and form #5828

Merged
merged 21 commits into from
Jun 25, 2024

Conversation

jp524
Copy link
Collaborator

@jp524 jp524 commented Jun 10, 2024

What github issue is this PR for, if any?

Resolves #5815.

What changed, and why?

Modified banners per issue description:

  • Rename values in 'Expires At' column
  • Add server-side and client-side validation to validate that expiry cannot be in the past
  • Validate the banner actually has content (done server-side only. No built in client-side validation exists for <trix> elements)
  • When a banner expires set it to inactive (I implemented this by modifying the existing Banner#expired? method. I'm open to feedback if there are better ways to implement this)

How is this tested? (please write tests!) 💖💪

Added some new model and system tests.

Screenshots please :)

New banner table
New banner table

Expires at validation
Expires at validation

Content validation
Content validation

@jp524 jp524 linked an issue Jun 10, 2024 that may be closed by this pull request
4 tasks
@github-actions github-actions bot added ruby Pull requests that update Ruby code Tests! 🎉💖👏 erb labels Jun 10, 2024
@jp524 jp524 requested a review from elasticspoon June 11, 2024 15:29
Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

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

Hey @jp524. Thanks for changes so far! Having poked around trying to make those systems tests work I think its better to just move those tests into request specs to simplify the testing.

app/views/banners/_form.html.erb Outdated Show resolved Hide resolved
spec/helpers/banner_helper_spec.rb Outdated Show resolved Hide resolved
spec/helpers/banner_helper_spec.rb Outdated Show resolved Hide resolved
spec/requests/banners_spec.rb Show resolved Hide resolved
spec/system/banners/new_spec.rb Outdated Show resolved Hide resolved
@jp524
Copy link
Collaborator Author

jp524 commented Jun 19, 2024

Thanks for your help @elasticspoon ! I implemented the changes and found a couple of bugs along the way so it was a good exercise.
Some of the CI workflows are failing but they don't give an actual error or test that is failing. I did run the test suite locally though and it all passes.

@jp524 jp524 requested a review from elasticspoon June 19, 2024 20:57
Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

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

Nice job dealing with all the time zone stuff. Its a pain in the ass and I'm glad I didn't have to do it.

Overall, I am happy with the PR. I had some minor nits but nothing blocking.

app/models/banner.rb Show resolved Hide resolved
spec/models/banner_spec.rb Outdated Show resolved Hide resolved
app/models/banner.rb Show resolved Hide resolved
spec/requests/banners_spec.rb Outdated Show resolved Hide resolved
@jp524
Copy link
Collaborator Author

jp524 commented Jun 23, 2024

Nits have been addressed! Should be good now @elasticspoon

@elasticspoon elasticspoon merged commit 2365e96 into main Jun 25, 2024
18 of 20 checks passed
@elasticspoon elasticspoon deleted the 5815-improve-banner-expiration-validations-and-form branch June 25, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
erb ruby Pull requests that update Ruby code Tests! 🎉💖👏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve banner expiration validations and form
2 participants