833497 - Support attachments in the reviewer tools

Status: VERIFIED FIXED

(Marketplace Graveyard :: Reviewer Tools, defect, P1)

Description

[Wil Clouser [:clouserw]]

This bug might need broken down further.

The reviewers have asked to be able to add attachments to new (note: not existing) comments.  

These will probably be in a new directory on disk.  Make sure to create it if it doesn't exist and add a permission check to the nagios page.  On the front end, the attachments should be behind reviewer auth and not come off the CDN since they might be private.

Here are some mockups (prepare to roll your eyes at google's UX):
> Desktop Upload: https://docs.google.com/file/d/0B96RwPRmGwk7QmIya09IUTVvNVU/edit
> Mobile Upload: https://docs.google.com/a/mozilla.com/file/d/0B96RwPRmGwk7Y3pJaG9XUnNDRHM/edit
> Desktop View: https://docs.google.com/a/mozilla.com/file/d/0B96RwPRmGwk7akduT2RKeWNQWjA/edit
> Mobile View: https://docs.google.com/a/mozilla.com/file/d/0B96RwPRmGwk7bUZQYUdvSWIxcWM/edit

I don't actually care too much about the desktop vs mobile view mockups - if it's a lot of work just use the mobile one for both.  Icons can be generic, no need to do actual previews.

Figure out something reasonable for file sizes - 5mb?

Comment 2

[Bram Pitoyo [:bram]]

It seems like the mockups were mislabeled, so here are the correct ones:

Displaying attachments, option 1:
https://docs.google.com/a/mozilla.com/file/d/0B96RwPRmGwk7bUZQYUdvSWIxcWM/edit

Displaying attachments, option 2:
https://docs.google.com/a/mozilla.com/file/d/0B96RwPRmGwk7akduT2RKeWNQWjA/edit

Uploading attachments, desktop:
https://docs.google.com/a/mozilla.com/file/d/0B96RwPRmGwk7QmIya09IUTVvNVU/edit

Uploading attachments, mobile:
https://docs.google.com/a/mozilla.com/file/d/0B96RwPRmGwk7Y3pJaG9XUnNDRHM/edit


To make things as simple as possible to implement, I propose that we use option 1 when displaying attachments. That is, stack them all vertically, in small text.

Comment 3

[Chuck Harmston [:chuck]]

Attachment: Attachment listing, in progress

In-progress screenshot attached for reference.

Icons cribbed from random various places in zamboni that had kinda-relevant icons. Feel free to suggest alternatives.

Comment 4

[Bram Pitoyo [:bram]]

(In reply to Chuck Harmston [:chuck] from comment #3)
> Icons cribbed from random various places in zamboni that had kinda-relevant
> icons. Feel free to suggest alternatives.

The only thing that would make this better is if there’s an icon that would indicate ‘a single page’ or ‘a single image’. Otherwise, all good. Thanks, Chuck!

Comment 5

[Chuck Harmston [:chuck]]

Do we want to allow an arbitrary number of attachments, or are we limiting it to 2? Screenshots indicate the latter, common sense the former.

Comment 6

[Bram Pitoyo [:bram]]

(In reply to Chuck Harmston [:chuck] from comment #5)
> Do we want to allow an arbitrary number of attachments, or are we limiting
> it to 2? Screenshots indicate the latter, common sense the former.

Hi Chuck. The screenshot was intended to visualize how attachments are displayed in the comments, rather than the actual number of attachments that are capable of being uploaded. The number is totally up to us, and I think that we shouldn’t limit it to 2.

Comment 7

[Lisa Brewster [:adora]]

http://www.quickmeme.com/meme/3t5qoi/

Comment 8

[Chuck Harmston [:chuck]]

This is ready for review:

https://github.com/chuckharmston/zamboni/compare/833497-reviewer_comment_attachment

@robhudson?

Comment 9

[Chuck Harmston [:chuck]]

This has been r+ed, but I'm holding back on merging until after the deploy tomorrow to make sure there's adequate time for testing a substantial patch.

Comment 10

[Chuck Harmston [:chuck]]

Landed: https://github.com/mozilla/zamboni/commit/6c0edc85ba6044c63a7caf59e46695ba2cb6b1cf

Comment 11

[krupa raj[:krupa]]

I filed bug 851193 but this seems to work.