1283024 - In finish-review screen, changing the review state reopens header box with added html tags

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: User Interface, defect, P1)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

STR:
1. Go to a review
2. Click "Finish Review..."
3. In the "Edit header" box, enter a couple of lines of text, e.g.: "bla" "foo"
4. Press "OK" to close box
5. Change review state (to anything different)

Expected:
Nothing, or header box reopens as it was before.

Actual:
Header box reopens, but html tags have been added! E.g.: "<p>bla<br />" "foo</p>".

Comment 4

[Mark Côté [:mcote]]

This keeps biting people.

Comment 5

[Mark Côté [:mcote]]

By the way, a workaround for this bug is to disable markdown.  You can do that by going to "My account" in the dropdown accessible by your login in the top right, then Settings, and uncheck "Always use Markdown for text fields".

Comment 6

[Daniel Holbert [:dholbert]]

That is not a complete workaround -- instead of HTML junk, I get URL-encoding junk (replacing <, >, ', ", and probably a bunch of other characters).

e.g. I just typed up some review feedback for bug 1269990, and then adjusted the review status to r+, and I was left with this in my header textbox (note all the &whatever; junk):

[...]
(1) typo: &quot;geomerty&quot;
(2) BUT, don&#39;t bother fixing (1), because that&#39;s the wrong term -- as you probably noticed in the spec (and the code-comment you&#39;re adding), the spec uses the term &quot;reference box&quot;. We should be using that term here too.
(3) &quot;handle...animation&quot; is a bit misleading here, since we don&#39;t actually do any useful animation in this case.  &quot;Refuse to interpolate&quot; is a bit more specific / accurate about describing the change.
[...]

Comment 7

[Daniel Holbert [:dholbert]]

(The "&whatever;" junk in comment 6 is particularly easy to trigger, given that reviewers are extremely likely to try to quote things (code, commit message, comments) when posting review feedback -- and quoting things will involve quote characters and/or ">" characters -- and this bug means that MozReview will stomp all over those.)

Comment 8

[David Walsh :davidwalsh]

The following line is a hack to keep the header textarea open but is causing the HTML issue:

$('#review-form-comments .body-top a.add-link').click();

So we need to find a better way to keep the textarea open without the fake click.

Comment 9

[David Walsh :davidwalsh]

I've identified a solution to prevent the HTML entity stuff; hoping to have a definitive fix in for this tomorrow.  The update will be in the reviewDialogView_mozreview.js file within our fork.

Comment 10

[David Walsh :davidwalsh]

Attachment: MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

Review commit: https://reviewboard.mozilla.org/r/71290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71290/

Comment 11

[:glob ✱]

Comment on attachment 8780606 [details]
MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

https://reviewboard.mozilla.org/r/71290/#review69016

::: pylib/mozreview/mozreview/static/mozreview/js/review_flag.js:61
(Diff revision 1)
>    save: function(){
> -    this.model.save({
> +      this.model.save({
> -      success: _.bind(function(){
> -        window.setTimeout(function () {
> -          $('#review-form-comments .body-top a.add-link').click();
> -        }, 0);
> +          attrs: ['extraData'],
> +          success: function() {
> +              if(RB.ReviewDialogView._instance) {
> +                  setTimeout(function() { RB.ReviewDialogView._instance._bodyTopView.openEditor(); }, 0);

this fixes the issue, however since we're touching this code can you please change it so the "editability" state of the field isn't changed when setting the review flag?

ie. if you've hit 'ok' to save the "header", changing the flag shouldn't make it editable again.

Comment 12

[David Walsh :davidwalsh]

Comment on attachment 8780606 [details]
MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

https://reviewboard.mozilla.org/r/71290/#review69016

> this fixes the issue, however since we're touching this code can you please change it so the "editability" state of the field isn't changed when setting the review flag?
> 
> ie. if you've hit 'ok' to save the "header", changing the flag shouldn't make it editable again.

I agree it should do that ultimately but since this fix seems somewhat urgent, I'd rather fix the core issue then make a UX improvement in a few days.

Comment 13

[:glob ✱]

Comment on attachment 8780606 [details]
MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

https://reviewboard.mozilla.org/r/71290/#review69016

> I agree it should do that ultimately but since this fix seems somewhat urgent, I'd rather fix the core issue then make a UX improvement in a few days.

ok

Comment 14

[:glob ✱]

Comment on attachment 8780606 [details]
MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

https://reviewboard.mozilla.org/r/71290/#review69110

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5758eee51317
MozReview: Prevent comment text from being mangled upon review state change. r=glob

Comment 16

[:glob ✱]

had to back this out:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/b7feebf9504f

under some circumstances this was preventing the review flag from being set.

1. load a review request where you are not a target reviewer
2. click finish review
3. change review state to r+

after a 'loading' spinner, the review state select is reset back to empty.


this issue isn't limited to impromptu reviews, heycam hit it on bug 1295895, but that's the easiest way to see it in action.

Comment 17

[:glob ✱]

Comment on attachment 8780606 [details]
MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

https://reviewboard.mozilla.org/r/71290/#review69870

as per https://bugzilla.mozilla.org/show_bug.cgi?id=1283024#c16 this doesn't always work.

Comment 18

[David Walsh :davidwalsh]

Attachment: MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

Review commit: https://reviewboard.mozilla.org/r/72294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72294/

Comment 19

[:glob ✱]

Comment on attachment 8782004 [details]
MozReview: Prevent comment text from being mangled upon review state change. (Bug 1283024)

https://reviewboard.mozilla.org/r/72294/#review70542

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/097405dc2105
MozReview: Prevent comment text from being mangled upon review state change. r=glob

Comment 21

[Daniel Holbert [:dholbert]]

I was just now bit by this yet again, over in bug 1296265 comment 2 (note the " junk in my comment there). You'd think I'd have learned by now, having hit this probably 5 times by now. :p

Is it unexpected that this is still being hit, when this bug is marked FIXED? (Is there a separate "deploy" step that needs to happen in order for the fix to show up in production MozReview?)

Comment 22

[Gregory Szorc [:gps]]

Yes, Pulsebot closes the bugs as soon as they land. We don't deploy to prod on Fridays. So this hasn't been deployed yet. My guess is glob will deploy on your Sunday night or early Monday morning.