1162111 - Publishing commit review requests directly is not prevented

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Comment 1

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: bz://1162111/smacleod

/r/8243 - MozReview: Cleanup unused CSS file (Bug 1162111); r=mcote
/r/8245 - MozReview: Hide draft banner buttons on commit requests (Bug 1162111); r=mcote

Pull down these commits:

hg pull -r b144820cd77ba1a93460485431226ba32c8bb623 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 2

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8245/#review6973

Shouldn't we disallow publishing commit rrs via the web API? Otherwise, this is merely (needed) wallpaper over an open barn door.

Comment 3

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/8245/#review6975

Yes, this is a WIP commit series. I put up the front-end stuff so that mcote could base off of it for his work. API fix is coming.

Comment 4

[Mark Côté [:mcote]]

Comment on attachment 8602204 [details]
MozReview Request: bz://1162111/smacleod

https://reviewboard.mozilla.org/r/8241/#review6979

Ship It!

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 8602204 [details]
MozReview Request: bz://1162111/smacleod

/r/8243 - MozReview: Cleanup unused CSS file (Bug 1162111); r=mcote
/r/8245 - MozReview: Hide draft banner buttons on commit requests (Bug 1162111); r=mcote
/r/8253 - MozReview: Stop allowing publish to commit requests (Bug 1162111); r=mcote
/r/8255 - MozReview: Hide problematic actions; r=mcote

Pull down these commits:

hg pull -r 86390adcbc90b6598dbaa98bbdb83e19a4f31a9d https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8602204 [details]
MozReview Request: bz://1162111/smacleod

/r/8243 - MozReview: Cleanup unused CSS file (Bug 1162111); r=mcote
/r/8245 - MozReview: Hide draft banner buttons on commit requests (Bug 1162111); r=mcote
/r/8253 - MozReview: Stop allowing publish to commit requests (Bug 1162111); r=mcote
/r/8255 - MozReview: Hide problematic actions; r=mcote

Pull down these commits:

hg pull -r 86390adcbc90b6598dbaa98bbdb83e19a4f31a9d https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8602204 [details]
MozReview Request: bz://1162111/smacleod

/r/8243 - MozReview: Cleanup unused CSS file (Bug 1162111); r=mcote
/r/8245 - MozReview: Hide draft banner buttons on commit requests (Bug 1162111); r=mcote
/r/8253 - MozReview: Stop allowing publish to commit requests (Bug 1162111); r=mcote
/r/8255 - MozReview: Hide problematic actions; r=mcote
/r/8261 - MozReview: Add test for preventing publish on commit requests; r=gps

Pull down these commits:

hg pull -r cfa9af3c0581e62755d28454c6e3ccab11601a7c https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 8

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8261/#review7029

LGTM.

Comment 9

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/8243/#review7041

Ship It!

Comment 10

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/8245/#review7043

Ship It!

Comment 11

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/8253/#review7045

Ship It!

Comment 12

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/8255/#review7047

Ship It!

Comment 13

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/8245/#review7049

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:43
(Diff revision 2)
> +#draft-banner {
> +  #btn-draft-publish,
> +  #btn-review-request-discard,
> +  #btn-draft-discard {
> +    display: none;
> +  }
> +}

Drive-by - instead of hiding the buttons by default, and special-casing the parent to show them - why not just hide them for the commit-request type?

Comment 14

[Mauro Doglio [:mdoglio]]

Comment on attachment 8602204 [details]
MozReview Request: bz://1162111/smacleod

https://reviewboard.mozilla.org/r/8241/#review7051

Ship It!

Comment 15

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/8245/#review7053

> Drive-by - instead of hiding the buttons by default, and special-casing the parent to show them - why not just hide them for the commit-request type?

In the event that our javascript breaks, or the structure changes so that the class is no longer an ancestor I wanted things hidden by default. Might be pointless though - thoughts on that?

Comment 16

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/8245/#review7057

> In the event that our javascript breaks, or the structure changes so that the class is no longer an ancestor I wanted things hidden by default. Might be pointless though - thoughts on that?

Ok, that makes sense.

We might want to land a hook in core to allow us to inject attributes on the review_request DIV that CSS can respond to.

Comment 17

[Steven MacLeod [:smacleod]]

Pushed https://hg.mozilla.org/hgcustom/version-control-tools/rev/2626181ab759

Comment 18

[Mark Côté [:mcote]]

Comment on attachment 8602204 [details]
MozReview Request: bz://1162111/smacleod

https://reviewboard.mozilla.org/r/8241/#review7127

Clearing review flag.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8602204 [details]
MozReview Request: bz://1162111/smacleod

I guess I no longer need to review this. Or maybe the flag didn't clear. I dunno.

Comment 21

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: MozReview: Stop allowing publish to commit requests (Bug 1162111); r=mcote

Comment 22

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: MozReview: Hide draft banner buttons on commit requests (Bug 1162111); r=mcote

Comment 23

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: MozReview: Cleanup unused CSS file (Bug 1162111); r=mcote

Comment 24

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: MozReview: Add test for preventing publish on commit requests; r=gps

Comment 25

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: MozReview: Hide problematic actions; r=mcote