-
Notifications
You must be signed in to change notification settings - Fork 58
feat(ExposureReport): RHINENG-20238 - Only allow selecting of the same major version #2435
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideLimits OS minor-version selection in the Exposure Report modal to the same major version as any already-selected version, by adding a helper to validate allowed majors and wiring it into the checkbox disabling logic. Flow diagram for checkbox disabling logic with major-version constraintflowchart TD
A[Render_minor_version_checkbox] --> B[Read_isSelected]
B --> C[Compute_selectedTotal]
C --> D[Check_isFetchLoading]
D --> E{Is_checkbox_currently_selected?}
E -->|Yes| F{Is_fetch_loading?}
F -->|Yes| G[Disable_checkbox]
F -->|No| H[Keep_checkbox_enabled]
E -->|No| I{Has_user_selected_any_version?}
I -->|No| J{Is_fetch_loading_or_selectedTotal_eq_5?}
J -->|Yes| G
J -->|No| H
I -->|Yes| K[Determine_osMajorVersion_for_this_checkbox]
K --> L[Call_isAllowedMajorVersion_with_osMajorVersion]
L --> M{Is_major_version_allowed?}
M -->|No| G
M -->|Yes| N{Is_fetch_loading_or_selectedTotal_eq_5?}
N -->|Yes| G
N -->|No| H
G:::disabled
H:::enabled
classDef disabled fill:#fdd,stroke:#d33,stroke-width:1px;
classDef enabled fill:#dfd,stroke:#3a3,stroke-width:1px;
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
e990112 to
a3f935e
Compare
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.
Hey there - I've reviewed your changes - here's some feedback:
- The
isAllowedMajorVersionhelper is quite hard to follow due to theObject.values(selectedOsVersions[0])[0][0]?.osMajorVersionaccess pattern; consider restructuringselectedOsVersionsaccess (or precomputing aselectedMajorVersionwithuseMemo) so the function reads as a straightforward comparison against a clearly named value. - Instead of recomputing the allowed major version check inline for each checkbox, you could compute the currently selected major version once per render and pass a simple boolean comparison into the
isDisabledexpression to reduce repeated work and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `isAllowedMajorVersion` helper is quite hard to follow due to the `Object.values(selectedOsVersions[0])[0][0]?.osMajorVersion` access pattern; consider restructuring `selectedOsVersions` access (or precomputing a `selectedMajorVersion` with `useMemo`) so the function reads as a straightforward comparison against a clearly named value.
- Instead of recomputing the allowed major version check inline for each checkbox, you could compute the currently selected major version once per render and pass a simple boolean comparison into the `isDisabled` expression to reduce repeated work and improve readability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
LightOfHeaven1994
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.
LGTM, pretty simple fix that works great!
a3f935e to
d92e43f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2435 +/- ##
==========================================
- Coverage 52.72% 52.64% -0.09%
==========================================
Files 166 167 +1
Lines 4681 4696 +15
Branches 1515 1463 -52
==========================================
+ Hits 2468 2472 +4
- Misses 1997 2224 +227
+ Partials 216 0 -216
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This updates the "Exposure report" modal to only allow selecting a minor version of the same major version already selected.
How to test:
Summary by Sourcery
New Features:
Summary by Sourcery
New Features: