feat(nimbus): setup results page with required data#13718
feat(nimbus): setup results page with required data#13718moibra05 merged 1 commit intomozilla:mainfrom
Conversation
mikewilli
left a comment
There was a problem hiding this comment.
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.
mikewilli
left a comment
There was a problem hiding this comment.
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.
a3d5782 to
38be646
Compare
mikewilli
left a comment
There was a problem hiding this comment.
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.
| { | ||
| "v3": { | ||
| "other_metrics": {"group": {"metricA": "Metric A"}}, | ||
| "metadata": { | ||
| "metrics": {"metricA": {"friendlyName": "Friendly Metric A"}}, | ||
| }, | ||
| "overall": { | ||
| "enrollments": {"all": {}}, | ||
| "exposures": {"all": {}}, | ||
| }, | ||
| "weekly": { | ||
| "enrollments": {"all": {}}, | ||
| "exposures": {"all": {}}, | ||
| }, | ||
| } | ||
| }, |
There was a problem hiding this comment.
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.
Because
This commit
Fixes #13682