-
Notifications
You must be signed in to change notification settings - Fork 34
Y25-286 - Add semantic meaning to data release timings #5491
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
Conversation
harrietc52
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just 1 minor comment I think - feel free to ignore.
| validates :data_release_timing, inclusion: { in: DATA_RELEASE_TIMINGS_FOR_OPEN_RELEASE }, | ||
| if: :data_release_timing_must_be_open? | ||
| validates :data_release_timing, inclusion: { in: [DATA_RELEASE_TIMING_NEVER] }, | ||
| if: :data_release_timing_must_be_never? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth adding some simple tests for this validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review 🙂
Validation tests can be found in study_spec.rb - no changes have been made, proving that the existing functionality is untouched.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5491 +/- ##
===========================================
+ Coverage 87.33% 87.38% +0.04%
===========================================
Files 1457 1457
Lines 32701 32703 +2
Branches 3422 3422
===========================================
+ Hits 28561 28577 +16
+ Misses 4125 4111 -14
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This refactoring was instrumental in and required for #5486
Changes proposed in this pull request
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main]- Check story numbers included
- Check for debug code
- Check version