499504 - Request For Information Replies do not appear in Addon Nomination page Item History

Status: VERIFIED FIXED

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

Description

[Zadkiel]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11
Build Identifier: N/A

If it doesn't have Author replies appearing in Item History the record of the Request for Information reply gets difficult to locate. Not having this record could cause issues. 

Reproducible: Sometimes

Steps to Reproduce:
Replicable[sometimes]
Steps:
- Request for more Information on Addon Nomination Page




Actual Results:  
- Author replies
- an email is sent to the Editor.

Expected Results:  
- Author replies
- an email is sent to the Editor
- reply appears on Addon Nomination Item History


Note: clicking the link associated to the request for information from the email prompts Editor to login again to AMO even though they already have.

Which is probably linked to 
Bug 483826 -  AMO not maintaining login

Comment 1

[Fred Wenzel [:wenzel]]

Do you have an example URL? The replies *should* show up, but it's of course possible that there's a bug in there somewhere.

Comment 2

[Zadkiel]

For sure.
Replies appear in the Item History
Review LoTW Search 20090314
https://addons.mozilla.org/en-US/editors/review/64462

Comment 3

[Fred Wenzel [:wenzel]]

Attachment: Show replies link

Did you click on the "Show/hide" link? (see screenshot)

Comment 4

[Zadkiel]

Oh, should...lol :P
 
Sorry Fred.
Review JSONovich 0.9.3
https://addons.mozilla.org/en-US/editors/review/63914

--------------Email that was sent to Editor -----------------
Michael J.  Giarlo has commented on version 0.9.3 of the add-on JSONovich:

"Sure, Zad.  It\'s this code, minified:\r\n\r\nhttp://code.google.com/p/google-code-prettify/source/browse/trunk/src/prettify.js\r\n\r\nWhat would be the best way for me to accommodate you?\r\n\r\nThanks,\r\n\r\n-Mike"

Please go to https://addons.mozilla.org/en-US/developers/discuss/15054 if you would like to reply.


If you have further questions, please e-mail [e-mail] or join #addons on irc.mozilla.org.

Mozilla Add-ons
https://addons.mozilla.org

--------------\Email that was sent to Editor -----------------

Oh yah...there's another bug with the escape characters show up.

Comment 5

[Fred Wenzel [:wenzel]]

(In reply to comment #4)
> Oh yah...there's another bug with the escape characters show up.

Yeah, that's bug 493463 :(

Comment 6

[Zadkiel]

Here's another one where the author responded 
and it doesn't show up in the item history.

Review firesymfony 1.1.0
https://addons.mozilla.org/en-US/editors/review/69443
https://addons.mozilla.org/en-US/developers/discuss/15091

Comment 7

[Zadkiel]

Here's another:

https://addons.mozilla.org/en-US/developers/discuss/15213
https://addons.mozilla.org/en-US/editors/review/68973

Comment 8

[Fred Wenzel [:wenzel]]

I can confirm this. Highly strange!

Comment 9

[Fred Wenzel [:wenzel]]

Want to hear something even stranger? I imported a production DB dump and on my local dev box, it works flawlessly for both add-ons in comment 6 and comment 7.

Comment 10

[Wil Clouser [:clouserw]]

(In reply to comment #9)
> Want to hear something even stranger? I imported a production DB dump and on my
> local dev box, it works flawlessly for both add-ons in comment 6 and comment 7.

What if you revert to productions revision?  Maybe we fixed something between now and then?

Comment 11

[Fred Wenzel [:wenzel]]

Thanks for the hint, I tried that but it still works fine. :( I'll read the code again in detail and try to figure out where it could break in production. I was going to say, maybe caching, but it does not make sense to be broken forever then.

Comment 12

[Fred Wenzel [:wenzel]]

This smells like a PHP problem. In the foreach() documentation, I read that references can behave strangely. I am going to write a patch avoiding referential changes to the history array.

Comment 13

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 1

This is as close as I can get to a solution right now, as I am not sure what's causing this (and that usually means, it's PHP's fault ;) ). Sadly, as I can't reproduce locally, I can only say it still works like it should, and this is totally PHP4-safe syntax now, no referential stuff that was only introduced in PHP5. I think we should give it a shot.

Comment 14

[Wil Clouser [:clouserw]]

Comment on attachment 390433 [details] [diff] [review]
Patch, rev. 1

Patch doesn't fix the problem.  Use the `remora` database to reproduce.

Comment 15

[Fred Wenzel [:wenzel]]

(In reply to comment #14)
> Use the `remora` database to reproduce.

Where?

Comment 16

[Wil Clouser [:clouserw]]

(In reply to comment #15)
> (In reply to comment #14)
> > Use the `remora` database to reproduce.
> 
> Where?

cm-webdev01-master01

Comment 17

[Fred Wenzel [:wenzel]]

(In reply to comment #14)
> Patch doesn't fix the problem.  Use the `remora` database to reproduce.

Thank you, I tried the production revision of the code on khan, along with the remora database, and it all works as expected. Still can't reproduce :(

Comment 18

[Wil Clouser [:clouserw]]

https://addons.mozilla.org/en-US/editors/review/69443 appears to be showing the replies, yes?

Comment 19

[Fred Wenzel [:wenzel]]

The bug in question may be: http://bugs.php.net/bug.php?id=35106 -- khan is running PHP 5.2.9 while locally I am running 5.2.5. Over at Debian (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=500679), I read this bug was fixed in PHP 5.2.1 -- now if we find out what's running in production, we may have the culprit.

Comment 20

[Fred Wenzel [:wenzel]]

reed confirmed, AMO is still running AMO 5.1.x and thus this is probably the root of the problem. I'll post a one-liner soon that'll work around this.

Comment 21

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 2

Here.

FWIW, my other patch would have fixed this too, but this is much more concise and points to the root of all evil[tm].

Comment 22

[Wil Clouser [:clouserw]]

Comment on attachment 390850 [details] [diff] [review]
Patch, rev. 2

r+ without being able to test it.  Bug 506703 is about upgrading PHP

Comment 23

[Fred Wenzel [:wenzel]]

r30507, thanks.

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Does comment 22 mean that this isn't testable on preview, either?

Comment 25

[raymond [:retornam] (inactive)]

verified fixed on https://preview.addons.mozilla.org/en-US/editors/review/70540?num=297#editorComments