Page MenuHomePhabricator

Upload Files action doesn't work
Closed, ResolvedPublic

Description

Attaching files to tasks is broken

I assume "Attached Files" in the property list is a special place for things like PDFs, DOCx, etc. Some minor nits that could make this more clear, consistent to users, easy to fix:

  • Upload File should likely be "Attach File"
  • I should be able to "Attach File" without leaving a comment
  • The preview text seems off.
  • There is no means to 'Attach File' during task creation (debatable, just an observation)

{F132900}

Event Timeline

chad raised the priority of this task from to Normal.
chad updated the task description. (Show Details)
chad added a project: Maniphest.
chad added subscribers: chad, epriestley, btrahan.

such cate

Ok "Upload File" action, should also work, which it seems it doesn't. haha

chad renamed this task from Upload Files action could be a bit more polished to Upload Files action doesn't work.Mar 21 2014, 9:03 PM

This doesn't work because of our javascript framework not submitting $_FILES correctly. They basically just get dropped. This is surprisingly not trivial to fix as uploading files is different than posting form data, though uploading files is part of a standard form post.

Fixing it can be done to work with most modern browsers but only IE10 or greater. (Something like http://blog.teamtreehouse.com/uploading-files-ajax) I don't think it would be *too* bad to implement, but requires doing it through the stack, hitting up JX.Workflow and JX.Request, and changing any "list of pairs" type data to a FormData object, thus breaking IE less than 10.

The most cross-browser compatible way to fix it I found is using an i-frame. Blech.

More practically, we can punt on solving this, resolve ourselves to not being able to upload files (other than the drag and drop upload), and just cut the 'attach file' feature from Maniphest altogether. FWIW I can't imagine this feature is used much anymore given it has been broken for who knows how long? If the whole "files are scattered throughout comments" thing is or becomes a problem maybe we can just add a list to the Task Details from all the comments?

This worked at one point though?

Before application transactions.

Well...

  1. Noone but me has complained but...
  2. Seems essential to be able to add files

Can we parse out non-image files and have them in property list?

Adding drag and drop seems fine.

More practically, we can punt on solving this, resolve ourselves to not being able to upload files (other than the drag and drop upload), and just cut the 'attach file' feature from Maniphest altogether.

+1, if you change the arrow-pointing-at-a-cloud button (an icon I have never seen and have no idea what it does until I hover over it) into something more standard, like a paperclip, which everyone's email client has. Once I saw the notice that I could drag-and-drop, I thought it was fine.

Should implement I think -

  • Upload File renamed to Attach File (Action : Upload File => Action : Attach File)
    • This includes this change in the Comments : Icons on mousever of the upload "cloud" icon ("cloud" : Upload File => "cloud" : Attach File)
  • Attach File action no longer has File : "Button Choose File" UI
    • ...instead, Attach File action has an instructions section that says "To attach a file, drag and drop it..." just like the Comments : Icons upload "cloud" icon dialog says
  • above is generally okay idea to help users understand this? we could also cut it

For transactions and property lists, options I see are -

  • All files referenced in comments - via Attach File UI or otherwise - generate "Attached file" transactions and get a link in the property list
  • The above, but only for non-images
  • No attached file transactions or property list at all

No attached file transactions or property list at all

I'm in favor of this. I think this kind of determines what we should do in other cases -- like, the root question here is: should Tasks have "Attachments", as distinct from files in comments / description?

I personally think the answer to this is "no", although I'm flexible on it if there are arguments for it. There's some discussion in T3742, but I think the answers to other things depend on this.

My thinking here is basically that it's pretty complicated (especially if we're going to commit to it and give users all the control they're likely to want out of it, like removing, updating, and reordering attachments) and not actually very useful. Tasks are rarely super long-lived, and don't usually have some big list of important files which are meaningful out of context. If they do, you can just put them in remarkup in the description, which gives you way more flexibility (you can order them, update them, have multiple lists, contextualize them, etc., and it's clear how to do all of it without more work on our part).

Basically, I think being able to put files in comments and other text areas is way more powerful and useful than an actual list of attachments on the task, and completely supersedes the use case.

Assuming the answer to that question of "separate attachments?" is "no", I think we should just do this:

  • Remove this action entirely.
  • Some day, make the upload dialog do iframe magic.
  • (Optionally, change icons / text / whatever.)

The only real reason this action exists right now is as a holdover from earlier days when attachments were a thing (which was before they had nice integration with Remarkup), and that a tiny number of users don't have drag-and-drop OSes (weird linux stuff) so it makes it more reasonable for them to attach files. I'm happy to throw them under the bus until we get around to iframe stuff (they can still use the old-school uploader in the Files application itself, or send email in some cases).

Attached diff kills the action.

I think if / when we ever add this back we should have "attached files" as a first class concept to get things like removing, reordering, updating, etc. across applications. Probably worth seeing if "Files" app needs some love at that time too with tighter policy integration and maybe per-file transactions? I dunno. Another day...!