Modular Transactions Migrations Errata
Closed, ResolvedPublic

Description

Macro

  • When creating a macro without uploading a file, a redundant / nonuseful "" is not a valid file PHID. message is emitted.
  • When creating a macro with no name, redundant errors are emitted ("must have a name" + " macro name must be at least three...").
  • Macros can be created using files which are not viewable images (like text files).
  • It is impossible to edit a macro and change simple fields (like "name" or "subscribers") without also uploading the file again (you get "Image macros must have a file").
  • Attaching audio to a macro does not appear to work at all: always "You must upload audio."
  • Macro feed stories are inconsistent about their use of the word "macro". For example, X disabled Image Macro "cat6". (no "macro") but X changed the image for macro Image Macro "cat6". (uses the word "macro"). For consistency with other applications and builtins (which use generic language so we don't have to translate X added a comment to <object>. for every object type), stories should omit the word "macro" if they can.

Fund

  • Important The "Can Create Merchants" permission in Phortune does not prevent users from creating merchants, even though it visually disables the "Create Merchant" action.
  • Important The "Can Create Initiatives" permission in Fund does not prevent users from creating initiatives, even though it visually disables the "Create Initiative" action.
  • Creating an initiative generates "Author set description / risks / merchant to ..." in addition to "Created". These initial state transactions should be suppressed, as they are in Differential and Maniphest. Oh, this is probably just because we aren't on EditEngine yet. So, uh, just ignore this. Or add a synthetic TYPE_CREATE transaction, like PhabricatorSlowvoteEditController to fix this in the short term.
  • Fund looks like it's still using old comment code (e.g., no stacked actions), but can probably be trivially upgraded.
  • Creating an initiative generates this feed story. Like the timeline event, this story should be suppressed (but is also an EditEngine thing, so ignore or use TYPE_CREATE to cheat for now):

  • The above story reads awkwardly ("changed the merchant receiving funds from I2 Cure Cancer initiative"). For consistency with Maniphest, Differential, etc, it should omit the word "initiative", and let "Xnnn Object Name" stand on its own.

Slowvote

  • The transaction for updating the description does not show the "Show Details" link to show a diff. This requires a little extra legwork in the Transaction, see Fund for examples.

Spaces

  • I failed to identify any issues with Spaces.

Legalpad

  • Creating a Legalpad document generates awkward transactions until EditEngin, like "X renamed this document from to Document Name."; we could smooth this over with an artificial TYPE_CREATE transaction.
  • Editing a document which requires signatures creates another "set the document to require signatures" transaction. We may be missing some casts. See also T12685.
  • Editing a "Corporate" document which does not require signatures, but which you created with signatures required, creates a "set the document to not require signatures" transaction. This is vaguely wrong but also probably pre-existing.

Passphrase

  • Destroying the secret for a credential and then assigning a new secret does not generate a transaction.
  • The "Description" field behaves like remarkup (for example, @username adds a subscriber), but does not use a Remarkup control. This is likely pre-existing.

Phame

  • Posts require a title, but do not have grey "Required" hint text in the empty form. This is likely pre-existing.
  • Blogs require a name, but do not have grey "Required" hint text in the empty form. This is likely pre-existing.
  • Slightly Important PhamePostTransaction has some old self:: constants which should be updated, in getMailTags().

Applications

  • PhabricatorApplication does return new PhutilMethodNotImplementedException instead of throw new. I'm pretty sure I suggested that this great code would work wonderfully as written. This results in this error, in the daemons (bin/phd debug taskmaster after making a configuration edit):
[2017-05-07 12:09:45] EXCEPTION: (Error) Call to undefined method PhutilMethodNotImplementedException::setActor() at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:69]
  • This can probably be fixed by returning new PhabricatorApplicationEditor().
  • Selecting "Save Policies" in policy configuration for applications without changing anything just reloads the page. It should redirect back to the application detail page. This is likely pre-existing.

EditEngine / Broader Issues

  • In Macro, when uploading a file and submitting the form with an error, the file is lost. This is a general issue with PhabricatorFileEditField and not trivial to resolve.
  • In Fund, as a merchant I can't refund a backer in the general case because I can't view the cart. I think? Unless I forgot about something magic. I think this is pre-existing and an artifact of Phortune being conservative about permissions.

Product / Minor

  • Macro has gained a "Subscribers" field on the create form. This is consistent, but perhaps not useful?
  • In Fund, the backer list has a default "no results" string ("No data found" instead of "no backers").
  • There is no UI feedback in Slowvote if you vote without selecting an option.
  • In Slowvote, "Reopen Poll" uses an unusual icon: . Other "reactivate" actions usually use .
  • In Slowvote, the "Open" tag in the header uses an unusual icon: . Other "open" states tend to use either or . @chad may have more insight into the rule here, if something more subtle exists.
  • In Slowvote, the "Status" checkboxes ("Open / Closed") in search have no label. They are fairly obvious without one, but in other applications they normally have a label.
  • In Slowvote, common and useful poll option "0" is not accepted as a valid response. This is likely a !$value check which should be a !strlen($value) check. Without this option, polls like "How many puppies should we adopt?" can lead to uncomfortable outcomes.
  • In Slowvote, attempting to vote in a closed poll (load form in one window, close poll in second window, submit form in first window) results in "400 Bad Request". Better would be UI guidance ("Voting has closed.").
  • Spaces does not publish feed stories, but reasonably could? Also mail.
epriestley updated the task description. (Show Details)May 7 2017, 5:13 PM
chad added a comment.May 7 2017, 5:15 PM

How do I remove subscribers from Macro, btw.

Just the input on the create form, not the feature wholesale, right? Let me finish up (just Ponder left) and then I'll dig it up, I'm pretty sure there's a method you can use for it but we don't do it very often.

Ponder

  • I couldn't realllly find any bona fide new issues with Ponder.

Ponder is A Bad Product

  • "Details" shows "no further details for this question" when empty, but this is inconsistent with other applications, like Maniphest, which show nothing.
  • On an open question, click Close Question. The default option is "Status: Open", i.e. not closing the question, i.e. not doing what the action said. Click "Submit", which is inconsistent with other applications, which tend to use tailored language for dialogs. Receive this exception:

  • The exception can be fixed with setContinueOnNoEffect(true) on the Editor which applies the change.
  • Mentioning users in answers does not subscribe them (oh, it does subscribe them to the answer).
  • Product language is inconsistent about whether the summary field is an "Answer Summary" (e.g., detail page) or an "Answer Wiki" (e.g., transaction history).
  • Answer history shows "udpated the answer details" instead of "wrote an answer" for the first transaction.
  • Answer history shows mentions adding subscibers to the answer which is nonsense? Except it isn't, since you can subscribe to an individual answer, we just don't show it anywhere.
  • Answer history only shows the monogram for the question as context for where you are or what you're looking at. Were we trying for real here, this page would maybe be better as "standalone permalink of the answer"? Maybe? But I think we had that and got rid of it? Who knows.
  • When you try to add an answer to a quesiton you have already answered (open two windows, answer in A, answer in B) you get no feedback (on the server: uncaught duplicate key exception).

Roughly:

  • Override willConfigureFields() in MacroEditEngine.
  • This fires after extensions add fields (so "Subscribers" will exist), but before form configuration acts (so users will still be able to turn the field on if they really want, by creating a form which enables it).
  • $fields is indexed by by field key. Ideally, we'd move 'subscriberPHIDs' to some constant on PhabricatorSubscriptionsEditEngineExtension, then use that, something like this:
if ($this->getIsCreate()) {
  $subscribers_field = idx($fields, PhabricatorSubscriptionsEditEngineExtension::FIELDKEY);
  if ($subscribers_field) {
    // By default, hide the subscribers field when creating a macro because it makes the
    // workflow SO HARD and wastes SO MUCH TIME.
    $subscribers_field->setIsHidden(true);
  }
}
  • Probably works?
chad added a comment.May 7 2017, 6:09 PM

How are you creating macros with non-viewable images?

shoppinglist

https://secure.phabricator.com/macro/view/190/

shoppinglist

Might be pre-existing. Source file is:

This is a genuine shopping list used by the Priestley household to purchase goods!

Oh, isViewableInBrowser() is the wrong check, and includes things like text files. Should be isViewableImage().

I've been terribly unlucky in locating fejolollololals in any grocery store here on the East "coast".

I couldn't find them either but I understand:

  • They are a kind of fruit.
  • They 100% exist and are real.
avivey added a subscriber: avivey.May 7 2017, 8:36 PM

I had a friend once which I think had a fejolollololal farm.

chad added a comment.May 8 2017, 8:12 PM

I can't reproduce "Attaching audio to a macro does not appear to work at all: always "You must upload audio.""... any ideas ?

Here's what I'm seeing locally in Safari:

  • Ready to submit form:

  • Click Upload File.
  • Error state after clicking the button:

I can dig into this locally if you did the same thing without any luck.

chad added a comment.May 8 2017, 8:23 PM

Yeah I tried wav and mp3, safari and chrome, can't get an error here.

I'll poke at it.

Oh, the error just wasn't clear to me. I thought it meant "You must upload audio." in the sense of "you did not upload anything, but needed to". It actually means "You uploaded a file, but the file isn't a valid audio file as far as Phabricator is concerned.".

I'll just shoot you a text fix.

(It looks like I wrote this string myself, in D7159.)

chad added a comment.May 8 2017, 8:28 PM

time well spent!

chad updated the task description. (Show Details)May 8 2017, 11:30 PM
chad updated the task description. (Show Details)May 9 2017, 4:03 PM
chad updated the task description. (Show Details)May 9 2017, 5:49 PM
epriestley updated the task description. (Show Details)May 14 2017, 8:34 PM
epriestley updated the task description. (Show Details)May 14 2017, 8:55 PM

I think this stuff remains:

In Fund, as a merchant I can't refund a backer in the general case because I can't view the cart. I think? Unless I forgot about something magic. I think this is pre-existing and an artifact of Phortune being conservative about permissions.

Just going to punt this for now until we have an actual need for it (e.g., iterate on Fund for some reason).

There is no UI feedback in Slowvote if you vote without selecting an option.
In Slowvote, attempting to vote in a closed poll (load form in one window, close poll in second window, submit form in first window) results in "400 Bad Request". Better would be UI guidance ("Voting has closed.").

Filed these in T12709 for the future.

Spaces does not publish feed stories, but reasonably could? Also mail.

Just going to punt this until someone cares, too.

Oh, the upload control ended up better but not great, but I'm not going to specifically file anything for that since I don't think we'd pursue it on its own and don't think we'll forget about it when we next do adjacent work.

epriestley closed this task as Resolved.May 14 2017, 10:11 PM
epriestley claimed this task.