Conversation
|
When testing locally with the "this is a rollout" checkbox checked, you'll hit 3 additional warnings not in the video for the population percent, total enrolled clients, and reference branch fields. A few thoughts on handling these:
Separately, we might need to bring over the AI risk field from the old UI into the new Risks card, and Public description into the Overview card or we can handle them the same way since we don’t currently have those fields. Let me know what you think! |
254cbe9 to
26798a0
Compare
* adds a setup completion progress bar to the rollout sidebar * adds an issues modal that lists invalid fields with links to the relevant card * allows rollouts to set 0% population, 0 enrolled clients, and no reference branch description * validates the rollout schedule (at least one phase, nonzero first phase) via NimbusRolloutReviewSerializer Fixes #15927
|
Update: instead of hard-coding some values to prevent the warnings mentioned previously, I added a dedicated NimbusRolloutReviewSerializer that the new UI uses (the old UI keeps the base serializer, so this is all scoped to the new UI). That clears the 3 warnings:
However, we may still want to add AI risk in the Risks card and Public description in the Overview card. |
# Conflicts: # experimenter/experimenter/nimbus_ui/templates/new/rollouts/audience/card.html # experimenter/experimenter/nimbus_ui/templates/new/rollouts/rollout_features/card.html # experimenter/experimenter/nimbus_ui/tests/test_new_views.py
Because * rollout authors need to know how much and what is left before a rollout can be launched This commit * adds a setup completion progress bar to the rollout sidebar * adds an issues modal that lists invalid fields with links to the relevant rollout card
yashikakhurana
left a comment
There was a problem hiding this comment.
Thank you @RJAK11 this is really great, nice approach to use separate serializer, and I have tested it locally, majorly works fine, some improvements to make, otherwise very good work
| tracked_fields = set() | ||
| for fields in self.SETUP_SECTIONS.values(): | ||
| tracked_fields.update(fields) | ||
| tracked_fields.update(self.NON_FIELD_ISSUE_CARDS) | ||
| tracked_fields.update(error_keys) | ||
|
|
||
| invalid_tracked = error_keys & tracked_fields | ||
| context["setup_completion_percent"] = round( | ||
| 100 * (len(tracked_fields) - len(invalid_tracked)) / len(tracked_fields) | ||
| ) | ||
|
|
There was a problem hiding this comment.
tracked_fields.update(error_keys) adds every erroring field into the denominator, which causes two issues:
error_keys & tracked_fields then always equals error_keys, so the & never filters anything (could just be len(error_keys)).
The total keeps changing — an unexpected error grows the denominator, so "100%" isn't a stable baseline.
Can we keep the denominator fixed to the known fields and only count broken ones?
tracked_fields = set()
for fields in self.SETUP_SECTIONS.values():
tracked_fields.update(fields)
tracked_fields.update(self.NON_FIELD_ISSUE_CARDS)
# don't add error_keys — keep total fixed
broken = error_keys & tracked_fields
percent = round(100 * (len(tracked_fields) - len(broken)) / len(tracked_fields))
One decision for us to make first: should an unexpected error drag the progress bar down, or just show up in the issues list without affecting the percent? With the fixed denominator it does the latter. Right now test_setup_progress_untracked_issue_lowers_completion expects the former — so let's agree on the intended behavior and make the code + test match. My vote is the bar tracks only known setup fields, and unknown errors just appear as issues.
| max_value=100.0, | ||
| required=True, | ||
| error_messages={"min_value": NimbusConstants.ERROR_POPULATION_PERCENT_MIN}, | ||
| ) |
There was a problem hiding this comment.
min_value=0.0 is what we want for rollouts, but you're still passing the old "must be >= 0.0001" message — it can never fire
| queryset=NimbusRolloutPhase.objects.all(), | ||
| ) | ||
|
|
||
| def validate_reference_branch(self, value): |
There was a problem hiding this comment.
can we add a comment here why we are simply returning the value for the future purpose
| SETUP_SECTIONS = { | ||
| "Overview": {*RolloutOverviewForm.Meta.fields}, | ||
| "Risks": {*RolloutRisksForm.Meta.fields}, | ||
| "Features": {*RolloutFeaturesForm.Meta.fields}, | ||
| "Audience": {*RolloutAudienceForm.Meta.fields}, | ||
| } | ||
|
|
||
| SECTION_CARD_IDS = { | ||
| "Overview": "overview", | ||
| "Risks": "risks", | ||
| "Features": "rollout-features", | ||
| "Audience": "audience", | ||
| } | ||
|
|
||
| NON_FIELD_ISSUE_CARDS = { | ||
| "rollout_phases": { | ||
| "section": "Rollout schedule", | ||
| "card_id": "schedule", | ||
| "label": "Rollout Schedule", | ||
| }, | ||
| } |
There was a problem hiding this comment.
SETUP_SECTIONS, SECTION_CARD_IDS, NON_FIELD_ISSUE_CARDS
All keyed by section, so easy to fall out of sync. Could we merge into one structure so a new section is added in one place?
Because
This commit
Fixes #15927
Screen.Recording.2026-07-08.at.3.01.38.PM.mov