347690 - Approval History for items in the Approval Queue

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, enhancement)

Description

[Justin Scott [:fligtar]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

It'd be nice to have a history of approvals/denials for each item so that we can make sure previous problems are fixed before approving.

Reproducible: Always

Comment 1

[Justin Scott [:fligtar]]

Attachment: patch of approval.php

patch for approval.php to add the link to Item History

Comment 2

[Justin Scott [:fligtar]]

Attachment: the new page: itemhistory.php

A new page for /developers... itemhistory.php

Comment 3

[Cameron]

Seems an excellent enhancement to me -> NEW

Also marking as blocking bug 335373. If you wanted, you could work on that instead/too, but this would be an excellent start towards fixing that bug :D

Comment 4

[Justin Scott [:fligtar]]

Hmm, the only thing mentioned in that bug that this doesn't do is show entries for new version submissions, and I had specifically excluded those so it wouldn't be cluttered. I can add it back.

By the way, you can test this at http://update-staging.mozilla.org/~fligtar

Comment 5

[Justin Scott [:fligtar]]

(In reply to comment #4)
> Hmm, the only thing mentioned in that bug that this doesn't do is show entries
> for new version submissions, and I had specifically excluded those so it
> wouldn't be cluttered. I can add it back.
> 
> By the way, you can test this at http://update-staging.mozilla.org/~fligtar
> 

After re-reading the bug, it mentions logging all edits to an extension (or so I assume). That's a bit more than just adding the new version submissions to this list, so I'm not going to make any changes to the patch I attached. If I end up working on the other bug, I'll post over there :)

Comment 6

[Cameron]

It seems I never did get around to establishing a relationship between these two. When you clarify the relationship (perhaps with Shaver) then you can dupe/resolve as necessary. 

Comment 7

[Justin Scott [:fligtar]]

Comment on attachment 232486 [details] [diff] [review]
patch of approval.php

This patch was included with the approval.php patch of bug 316930

Comment 8

[Michael Morgan [:morgamic]]

Comment on attachment 232487 [details]
the new page: itemhistory.php

When assigning from $_GET['id'] -- instead of populating a variable with exactly what was in $_GET then using that variable later with escaping around it, you should escape when doing the original assign -- so you don't wind up accidentally trusting the variable.

Will commit this with that change for variable assignment added.

Comment 9

[Michael Morgan [:morgamic]]

Comment on attachment 232487 [details]
the new page: itemhistory.php

Couple of other notes:
* Inline styles ideally wouldn't be used, and definitely not used to define styles twice on one element (td).  Next time it'd make sense to edit the CSS files instead.
* Jumping in and out of PHP while using loops is pretty messy, and you might be better off using echo statements or heredoc format (<<<FOO ... {$var} ... FOO;).
* Also, indentation was a bit off -- we usually gravitate towards PEAR standards (4 spaces, etc.)

Given the above, I'm stil taking the patch because it works, and I'm not going to nitpick cosmetic stuff -- the help is much appreciated.