Closed Bug 493463 Opened 15 years ago Closed 13 years ago

Developer replies for info requests are left escaped

Categories

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P4)

Tracking

(Not tracked)

RESOLVED FIXED
4.x (triaged)

People

(Reporter: Gijs, Unassigned)

Details

STR:

1. Ask developer for info on an add-on submission.
2. Developer replies, including at least one newline.
3. Open review pane for this add-on, and un-collapse the reply.

Actual results:
https://addons.mozilla.org/en-US/editors/review/57675?num=4

"Hi Gijs,\r\n\r\nyou can access the entire repository of the project via:\r\n\r\nhttp://svn.terragate.net/svn/noxss/ \r\n\r\nThe repository will be publicly announced on noxss.org as soon as I made it available via svn.noxss.org\r\n\r\nCheers,\r\nJeremias\r\n"

Expected results:
"Hi Gijs,

you can access the entire repository of the project via:

http://svn.terragate.net/svn/noxss/

The repository will be publicly announced on noxss.org as soon as I made it available via svn.noxss.org

Cheers,
Jeremias
"
Priority: -- → P4
Target Milestone: --- → 4.x (triaged)
I'd be happy to look into fixing this myself if someone could give me a hint at what needs to be done here. :-)
This may have to do with Markdown (the thing we're using on the editor pages instead of HTML).  I'd like to replace it all together with HTMLPurifier so it matches what we're doing on the public pages.  That's part of the problem - it's inconsistent, so without digging into it I'm not sure what the problem is.  I'm guessing it's in the editors_controller.php in a function called review() though.
(In reply to comment #2)
> This may have to do with Markdown (the thing we're using on the editor pages
> instead of HTML).  I'd like to replace it all together with HTMLPurifier so it
> matches what we're doing on the public pages.  That's part of the problem -
> it's inconsistent, so without digging into it I'm not sure what the problem is.
>  I'm guessing it's in the editors_controller.php in a function called review()
> though.

OK, so after looking into this, here are some notes:

1) (Developer?) Comments are stored in the DB with the \r\n escaped.
2) Editor comments are displayed with the newlines intact, but not replaced with <br> or equivalent, so all the text ends up run together (as HTML doesn't care about the kind of whitespace it gets) but not with literal "\r\n" in the text.
3) Developer comments are displayed  with literal \r\n in the text, both on the discuss page and on the review page.
4) Wil said the developer comments are stored escaped (w/ literal \r\n in the text).
5) The notification email the editor gets for the developer reply also features literal "\r\n". :-(

I looked at the review function, but don't think just modifiying that solves all our problems (mostly because the issues show up in the email and the discuss page as well). Instead, I think we should store the developer reply with a "normal" newline (like the editor one), and on display replace the newlines in any review with <br> (which could be a followup bug). It wouldn't fix the old discussions, I guess, but at the present rate it wouldn't be long until they're not really useful anymore - and if necessary I'm guessing we could run a SQL query to fix them. Does that make sense?
(In reply to comment #3)
> Instead, I think we should store the developer reply
> with a "normal" newline (like the editor one), and on display replace the
> newlines in any review with <br> (which could be a followup bug). 

Storing input with actual newlines is definitely correct.  Anything else is a bug (in this case, storing a literal \n).  Once they are stored correctly, printing them is easily done by running them through nl2br().
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.