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

VERIFIED FIXED in 5.0.8

Status

P3
major
VERIFIED FIXED
10 years ago
3 years ago

People

(Reporter: zadkiel.m, Assigned: wenzel)

Tracking

unspecified
5.0.8

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
(Assignee)

Comment 1

10 years ago
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.
(Reporter)

Comment 2

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

Comment 3

10 years ago
Created attachment 384258 [details]
Show replies link

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

Comment 4

10 years ago
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 amo-editors@mozilla.org 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.
(Assignee)

Comment 5

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

Yeah, that's bug 493463 :(
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 6

10 years ago
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
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
(Assignee)

Comment 8

10 years ago
I can confirm this. Highly strange!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → 5.0.8

Updated

10 years ago
Severity: normal → major
Assignee: nobody → fwenzel
Priority: -- → P3
(Assignee)

Comment 9

10 years ago
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.
(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?
(Assignee)

Comment 11

10 years ago
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.
(Assignee)

Updated

10 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 12

10 years ago
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.
(Assignee)

Comment 13

10 years ago
Created attachment 390433 [details] [diff] [review]
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.
Attachment #390433 - Flags: review?(clouserw)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Attachment #390433 - Flags: review?(clouserw) → review-
Comment on attachment 390433 [details] [diff] [review]
Patch, rev. 1

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

Comment 15

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

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

cm-webdev01-master01
(Assignee)

Comment 17

10 years ago
(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 :(
https://addons.mozilla.org/en-US/editors/review/69443 appears to be showing the replies, yes?
(Assignee)

Comment 19

10 years ago
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.
(Assignee)

Comment 20

10 years ago
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.
(Assignee)

Comment 21

10 years ago
Created attachment 390850 [details] [diff] [review]
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].
Attachment #390433 - Attachment is obsolete: true
Attachment #390850 - Flags: review?(clouserw)
(Assignee)

Updated

10 years ago
Attachment #390850 - Attachment is patch: true
Attachment #390850 - Attachment mime type: application/octet-stream → text/plain
Attachment #390850 - Flags: review?(clouserw) → review+
Comment on attachment 390850 [details] [diff] [review]
Patch, rev. 2

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

Comment 23

10 years ago
r30507, thanks.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: push-needed
Resolution: --- → FIXED
Does comment 22 mean that this isn't testable on preview, either?
verified fixed on https://preview.addons.mozilla.org/en-US/editors/review/70540?num=297#editorComments
Status: RESOLVED → VERIFIED
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.