Fix rollout timeline/pop page fixes #2403 #2406
Conversation
uhlissuh
commented
Mar 19, 2020
uhlissuh
commented
| @@ -94,6 +96,49 @@ class TimelinePopForm extends React.PureComponent { | |||
| } | |||
| } | |||
|
|
|||
| displayRolloutPlaybookOrEnrollment() { | |||
There was a problem hiding this comment.
I'd rather split this into two things like
displayRolloutPlaybook
displayEnrollmentDuration
and then inside each one just start with the bool check about whether it should show
| <option>Rollout Playbook</option> | ||
| <option>Low Risk Schedule</option> | ||
| <option>High Risk Schedule</option> | ||
| <option>Marketing Launch Schedule</option> | ||
| <option>Custom Schedule</option> |
There was a problem hiding this comment.
Oh shouldn't these be
- in the js constants file
- those are the display strings but don't the options also need the string values (the lower case ones) here? https://github.com/mozilla/experimenter/blob/master/app/experimenter/experiments/constants.py#L25-L35
There was a problem hiding this comment.
@jaredlockhart you'd like to see if more how I did the firefox versions?
There was a problem hiding this comment.
Because I just put the value into every option and that works, however I can refactor it to look more like firefox versions, where there's a function that format's each option withwhat's in the JS constants file.
There was a problem hiding this comment.
Oooohhh I see what's happening here.
The fields in the database are charfields, so they'll accept anything, so anything you put in there is valid and will just get saved and displayed.
Usually in Django world you define two things, a db value and a display value, and then define the pair as CHOICES, example:
Then later when you're dealing with the field in code somewhere, you're comparing against the db value not the display value.
We didn't notice because a bunch of these things just only stay as text and we never refer to teh values in code, we just take wahtever it is and display it on the detail page, and the db values and display values are the same, like for platform:
So yes exactly it should be like the way versions are done, and actually the real fix here is to change the serializer to use a ChoiceField instead of a CharField so it actually checks that the values are from the list we expect, but I think we can do that as a followup ticket so we don't blow this one up.
There was a problem hiding this comment.
@jaredlockhart ok I've made the change to where the playbook options are in constants and then they are displayed with the function displayOptions, which is a generalized version of a function that used to just be formatting the versions options... but is now formatting versions and playbook options.
|
now you can see that high-risk is being saved correctly in DB and displaying the right graph. |
There was a problem hiding this comment.
Awesome just tried locally and works perfectly, thank you for the fast fix and updates from feedback, the displayOptions is very clean and elegant, I love it! Great job @uhlissuh 😄 🎉
* Fix rollout timeline/pop page fixes mozilla#2403 * Split into two methods of enrollment and playbook * Add values to options Co-authored-by: Jared Lockhart <119884+jaredlockhart@users.noreply.github.com>
