Skip to content

Comments

feat(nimbus): setup results page with required data#13718

Merged
moibra05 merged 1 commit intomozilla:mainfrom
moibra05:13682
Oct 24, 2025
Merged

feat(nimbus): setup results page with required data#13718
moibra05 merged 1 commit intomozilla:mainfrom
moibra05:13682

Conversation

@moibra05
Copy link
Contributor

Because

  • Results page view context requires specific data to render contents

This commit

  • Passes the required context data through the view

Fixes #13682

Copy link
Contributor

@mikewilli mikewilli left a comment

Choose a reason for hiding this comment

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

I left some suggestions inline, but overall it's looking pretty close.

One thing to be aware of is that the schemas/ folder actually contains its own python package that is published separately from Experimenter. This means you'll want to update and publish a new version in a separate PR if you want to make changes to it (you can see the schemas/README file for docs on this). However, see my comments inline there for a potential alternative approach.

Copy link
Contributor

@mikewilli mikewilli left a comment

Choose a reason for hiding this comment

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

Let me know if you want to discuss my concerns, maybe I'm misinterpreting what will happen here or maybe we just need to figure out a better way to approach this.

It also might be appropriate to split this out into two PRs that separates the client changes from the UI changes.

@moibra05 moibra05 force-pushed the 13682 branch 2 times, most recently from a3d5782 to 38be646 Compare October 23, 2025 17:09
Copy link
Contributor

@mikewilli mikewilli left a comment

Choose a reason for hiding this comment

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

Ok I left too many comments, this is largely good as-is. My main concern is with the has_exposures logic, everything else is relatively small suggestions.

Copy link
Contributor

@mikewilli mikewilli left a comment

Choose a reason for hiding this comment

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

LGTM thanks! :shipit:

Comment on lines +2994 to +3009
{
"v3": {
"other_metrics": {"group": {"metricA": "Metric A"}},
"metadata": {
"metrics": {"metricA": {"friendlyName": "Friendly Metric A"}},
},
"overall": {
"enrollments": {"all": {}},
"exposures": {"all": {}},
},
"weekly": {
"enrollments": {"all": {}},
"exposures": {"all": {}},
},
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

If you wanted to be more concise, you could keep the static parts in the function and only put the dynamic stuff (the parts that go in weekly and overall) in here. This is OK for now though.

@moibra05 moibra05 added this pull request to the merge queue Oct 24, 2025
Merged via the queue into mozilla:main with commit f9bcb01 Oct 24, 2025
16 checks passed
@moibra05 moibra05 deleted the 13682 branch October 24, 2025 20:00
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.

Prepare Django results view context for migration from React

3 participants