Skip to content
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

Peer review #287

Closed
1 task
mythmon opened this issue Oct 4, 2016 · 8 comments
Closed
1 task

Peer review #287

mythmon opened this issue Oct 4, 2016 · 8 comments

Comments

@mythmon
Copy link
Contributor

mythmon commented Oct 4, 2016

Normandy should require two different people to sign off on a recipe before it is signed and can be enabled.

Blocked by:

@mythmon mythmon added the Epic label Oct 4, 2016
@Osmose Osmose removed the Epic label Oct 4, 2016
@mythmon mythmon self-assigned this Oct 4, 2016
@Osmose Osmose added this to the October 2016 Sprint milestone Oct 7, 2016
@Osmose Osmose removed this from the October 2016 Sprint milestone Nov 1, 2016
@Osmose
Copy link
Contributor

Osmose commented Nov 18, 2016

Mockups!

Peer Review Draft 3.pdf

Major changes implied by this design:

  • Publishing any recipe requires approval, even if an approved revision was disabled. This is partially to help implementation, and partially because publishing a recipe is a product decision to some extent. The alternative would involve either being able to re-activate a recipe in 6 months without approval, or having odd timeouts on required approval, which is an unwieldy solution.
  • The dedicated history review is replaced by a sidebar.
  • The mockups show comments for each revision, which is covered by Allow user-entered comments for revisions #333, but depending on when that lands vs this, the revision list might only have numbers at first. We'll figure out a sane way to handle this however it plays out.

@mozilla/strategy-and-insights @mozilla/normandy Feedback? Changes?

@jvehent You might be interested in checking out the mockup as well.

@ghost
Copy link

ghost commented Nov 18, 2016

This is really nice, and I agree with the publishing flows that you described.

Can you expand on the "Recipe Listing" page? What revision state info do we publish to the list? Right now it looks like "Test Log" is shown twice with different statuses, but that's not enough for me to understand the logic.

I think I'd want a list that shows order of important revision states for a given recipe. Perhaps "Status" should show the order of the most recent of each status type with the following constraints:

  • "Draft" only appears if it's the most recent status, or else we leave it out since it's likely abandoned
  • "Review" can only occur more recently than "Published", or else we leave it out since it's likely abandoned

This makes our potential Statuses the following list:

  • "Draft" - a draft initialized, there is no published version or review version
  • "Draft" < "Published" - a draft initialized after the last publish
  • "Draft" < "Review" - a draft initialized after the last review, there is no published version
  • "Draft" < "Review" < "Published" - a draft initialized after the last review which occurred after the last publish
  • "Review" - a review initialized, there is no published version
  • "Review" < "Published" - a review initialized after the last publish
  • "Published" - a publish initialization was the most recent action

Feel free to disregard, maybe you have a better mental model for how you want to arrange multi-state recipes.

Minor suggestion: I prefer single word states, "Review" > "In Review"

@Osmose
Copy link
Contributor

Osmose commented Nov 18, 2016

Can you expand on the "Recipe Listing" page? What revision state info do we publish to the list? Right now it looks like "Test Log" is shown twice with different statuses, but that's not enough for me to understand the logic.

That's purely me forgetting to change the name of the recipe. The status column reflects the state of the current-most revision for the recipe.

I think I'd want a list that shows order of important revision states for a given recipe. Perhaps "Status" should show the order of the most recent of each status type with the following constraints: <snip>

I think we can cover the possible states with two columns more easily than with one. It'd also make filtering a little more robust.

What if we had a second column indicating published status, such that seeing "Draft" status and a checkmark for "Published" indicates "There is currently a published revision being sent out, and there are unpublished drafts as well"?

Minor suggestion: I prefer single word states, "Review" > "In Review"

👍

@ghost
Copy link

ghost commented Nov 19, 2016

That's a cleaner interface for accomplishing my request. Sounds good

@jvehent
Copy link
Contributor

jvehent commented Nov 19, 2016

/me likes 👍

Would it be better to have separate tabs for recipes that are in Draft, Pending Review, Published and Done? I'm thinking it might be easier to sort through that filtering on the status column. Minor comment thought, feel free to ignore.

@ghost
Copy link

ghost commented Nov 19, 2016

I'm leaning towards a global list for a view of whole system state. Especially since we have the filters to emulate the alternative tab views

@mythmon
Copy link
Contributor Author

mythmon commented Nov 19, 2016

Although the mockup has the listing page on it, I think we should focus more on the detail page. The listing page is something we'll be working on separately, and likely won't be a part of this change.

@Osmose
Copy link
Contributor

Osmose commented Apr 26, 2017

v1 of peer signing landed in #613. Future improvements and tweaks will be separate issues.

@Osmose Osmose closed this as completed Apr 26, 2017
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

No branches or pull requests

3 participants