Page MenuHomePhabricator

Use keywords instead of ints to update task priority in ManiphestEditEngine
ClosedPublic

Authored by amckinley on Jun 9 2017, 10:52 PM.

Details

Summary

Fixes T12124. Changes ManiphestEditEngine to populate the select using priority keywords instead of the integer value. Marks maniphest.querystatuses as frozen. Adds a new Conduit method for fetching potential task statuses.

Test Plan

Created tasks and changed their priorities, observed that transactions in the DB still have the same type (integers as strings). Invoked maniphest.update with priority => '90' and observed that it still works. Invoked maniphest.edit with priority => 'unbreak' and observed that it now works.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

amckinley created this revision.Jun 9 2017, 10:52 PM

There are a few other callsites that create task priority transactions that still need to be cleaned up. Hopefully nothing else sets task priorities without going through the transactions layer.

amckinley updated this revision to Diff 43571.Jun 9 2017, 11:31 PM
  • more callsites for priority changing
epriestley requested changes to this revision.Jun 12 2017, 5:16 PM
  • One actual bug inline, I think.

I think this changes behavior in this case:

Setup:

  • Add a new priority X to the config.
  • Set a task to priority X.
  • Remove that priority from the config.

Test:

  • Click "Edit Task" on the task.
  • Change only the description.
  • Save the task.

Expect/Old Behavior:

  • The task priority is not touched.

Actual/New Behavior:

  • The task priority is set to '' (empty string).

This is caused by the new 'unknown' thing.

A not-so-great fix would be to hard-code some string like "unknown" as magic (although maybe pick a keyword which no user could ever reasonably select) that means "do nothing", then have generateNewValue() just return the current value if the new value passed in is the magic "do nothing" value. That isn't great but retains the behavior and doesn't seem too awful?

src/applications/maniphest/command/ManiphestPriorityEmailCommand.php
55

Should be if ($priority === null)?

This revision now requires changes to proceed.Jun 12 2017, 5:16 PM

Yeah, I'm not a fan of the way I changed the "current status no longer exists" code. In general I'm starting to feel like this change is too big and already too magical. Are there other transactions that work similarly to how ManiphestPriorityTransactions would work after this diff? Where constants get turned into different representations of the same constants when they get serialized?

I'm wondering if maybe I could just change maniphest.edit to translate from the incoming symbolic names back to the integer representation before invoking the edit engine stuff. Is there any way to hook into PhabricatorEditEngineAPIMethod to twiddle the request arguments?

I think the "current status no longer exists" is the biggest wart on this stuff (notwithstanding the last few call sites that use ManiphestPriorityTransactions that I haven't migrated yet). Maybe we should just yell at the user when they try and edit a task with a priority that doesn't exist any more? Or silently change the priority back to the default priority? Or refuse to accept a new config for priorities that removes a priority assigned to existing tasks? (That last one seems like a lot of work, but might be the cleanest approach. We could even pre-generate a bulk edit job that changes all tasks with the removed priority to something else). Doing "extreme vetting" on the priority config would also prevent the nightmare scenario of "priority gets removed from the config, time passes, someone comes along and adds a new brand new unrelated priority to the config and accidentally re-uses the original priority int",

We can't really do the last one ("prevent this state") because users can edit config.json and we can't prevent them from saving the file. On startup, we could test if any task had an invalid state and refuse to continue if it did, but we can't do this on every page load and you can edit config (or edit the database) between page loads. It would also mean that you could lock your entire install up with one bad row import or whatever. And I don't think it generalizes well: we can't universally prevent invalid states from existing (for example, we can't reasonably guarantee that every PHID of every object corresponds to a real PHID, or that no edge graphs have cycles, etc., because these things are incredibly expensive to check).

We could prevent the edit ("You must select a valid priority") but I think this isn't a great fix on the balance. In particular, there are many cases where we use the other rule already ("editing an object with invalid fields is OK if you don't change the fields"), and a number of cases where this is more clearly the right behavior. An example is that a revision might have reviewers (projects or Owners packages) who you can't see. We let you edit the revision, save it, add new reviewers, and remove those reviewers: all of those are desired/expected. You just can't add new reviewers you can't see. If we started changing this rule more broadly, editing a revision which had triggered review by #secret_securtiy_cabal would fail and require you to remove #secret_security_cabal to continue.

We could make the Conduit API keyword-based and the internal API numeric, but that will cause stuff like this to go weird:

https://secure.phabricator.com/maniphest/task/edit/form/4/?priority=100

That currently prefills the form with "High" priority, but you have to use 100. After this change, you can do priority=high instead, which I think is much better. If we did a weird "external vs internal" thing this would stay as priority=100, with priority=high not working.

Instead of doing magic, we could accept either a numeric value or a keyword value, then ban numeric keywords (if we don't already). Then the input could just get the numeric value as the custom key. That would create a little weirdness with Conduit, (where "100" is okay), but we could just surgically get rid of that with an explicit test, even ($this->getEditor()->getContentSource() and test that against being ContentSourceConduit or whatever).

This kind of field is very unusual, and possibly completely unique. I believe that in every other case values are either strings already or are numbers but we'd prefer they were strings.

Then the validation could look like this:

if (the new value is a keyword already) {
  great;
}

if (content source is conduit) {
  throw new validation error: use one of the keywords via conduit: high, normal, low, wishlist.
}

if (is numeric value) {
  fine;
}

throw new validation error: use a keyword: high, normal, low, wishlist

That could probably be organized better.

Then in getNewValue(), just:

if (is numeric already) {
  return value;
}

return get the numeric value (keyword);

We need a new UserShotOwnFootDetectorDaemon. How hard could it be to enumerate all possible bad states and test for them?

I'll probably go down the road of making the narrowest fix: new magic keyword constant that ManiphestTaskPriority treats as a no-op, with a nice comment explaining the weird behavior.

Oh, another thing: currently you can have multiple keywords associated with a single priority. When converting from int to keyword, I'm blindly using the first keyword in the list, which seems reasonable but I wanted to call attention to that behavior in case it breaks something. See ManiphestTask->getPriorityKeyword() for an example.

One more suggestion: maybe we should run a migration to change the type of ManiphestTask.priority to string and migrate ManiphestPriorityTransaction old and new values to string? Changing ManiphestTask.priority is probably a non-starter because we use the ints for sorting, but leaving it inconsistent with the transactions serializations also feels like a mistake. That's also a pretty huge change just for the sake of smoothing out some rough edges.

I think using the first keyword is reasonable and couldn't come up with anything it breaks.

If we stored it, users could shoot themselves a little bit with a sufficiently convoluted sequence of edits, like:

  • Rename "High" to "Important", keep keyword "high", add keyword "important".
  • Later, create a new priority "High", move keyword "high" to that.
  • Uhoh.

This seems fairly silly and like users are kind of asking for it? And also easy enough to walk them through recovering if necessary.

That's also a pretty huge change just for the sake of smoothing out some rough edges.

I think we prrrrobably should do this type-sanitizing stuff eventually, but maybe collect other errors we'd like to fix (T2968 has some too) and do them all at once so the total amount of pain involved is reduced at least a little bit. We approximately have until we ship transactions.search to fix it all, and that might not be in the terribly-immediate future.

We could also add, say, a priorityKey string field and store "high" there, and then leave priority with the corresponding integer value which was only used for sorting. But that's really just pure polish-on-polish if everything else works properly already.

amckinley updated this revision to Diff 43599.Jun 13 2017, 7:44 PM
amckinley edited edge metadata.
  • added new constant for representing priority keywords that dont exist any longer
  • added validation for priority keywords thats identical to status constant validation (alphanumeric, 1-12 characters)
  • marked priority keywords as required
amckinley updated this revision to Diff 43600.Jun 13 2017, 8:12 PM
  • fixing more callsites
  • removing isNewObject() check in ManiphestTaskPriorityTransaction which was breaking assigning priorities to new tasks
amckinley marked an inline comment as done.Jun 13 2017, 8:14 PM

This is every callsite that I could find (by grepping for ManiphestTaskPriorityTransaction::TRANSACTIONTYPE). I heavily tested the situation where a user wants to edit a task that's had its priority removed from the config. Also, see attached screenshot of the new keyword validation code in action:

I couldn't figure out how to test the changes to these classes (but I could probably get email working enough to test email commands and herald actions):

  • ManiphestPriorityEmailCommand
  • ManiphestSubpriorityController
  • ManiphestTaskPriorityHeraldAction
  • PhabricatorProjectMoveController
epriestley accepted this revision.Jun 13 2017, 8:49 PM

Couple inlines.

src/applications/maniphest/conduit/ManiphestStatusSearchConduitAPIMethod.php
40

isset() returns true if the value exists but is set to false. A better test is probably !empty(...), since I would expect 'closed' => false to mean "not closed".

src/applications/maniphest/constants/ManiphestTaskPriority.php
133–141

Perhaps this is too strict? I don't think there's any reason that a Chinese-language install can't use "keywords": ["important", "重要"] (which is what Google says is Chinese for "important"), or an install can't use a 13-letter keyword.

This also allows 0-length keywords, which probably are invalid.

Maybe something like: minimum of 1 byte, maximum of 64 bytes, no spaces, not entirely digits (in case we want to interpret priority numbers as magic later)?

This revision is now accepted and ready to land.Jun 13 2017, 8:49 PM
amckinley added inline comments.Jun 13 2017, 9:29 PM
src/applications/maniphest/conduit/ManiphestStatusSearchConduitAPIMethod.php
40

Okie dokie. In practice, the only statuses that had anything set for closed had closed => true, but I'll fix it.

src/applications/maniphest/constants/ManiphestTaskPriority.php
133–141

Should I change ManiphestTaskStatus->isValidStatusConstant() similarly? That's where I pulled this code from.

(Oh, the regexp has + in it so it doesn't accept empty string.)

In the case of "status", we have a separate string key (like "open") and then a list of keywords (not currently validated). I guess I'd say:

  • Status and priority keywords should have the same validation rules, probably: 1-64 bytes, no spaces, not completely numeric?
  • The status string key is currently stored in a text12 field on ManiphestTask. If we put "up to 64 bytes" validation on it, we should change that to text64. I think this is reasonable, the "12" is probably just a legacy carryover from none of the original hard-coded statuses being longer than that ("duplicate" is 9?). But we can't make that validation more flexible without changing the field, or 13+ byte strings won't actually work.

And I can't come up with any real reason not to let users use "🐐🐐🐫🐪" as their primary stored key in the database, so I think weakening the validation is otherwise fine.

And I can't come up with any real reason not to let users use "🐐🐐🐫🐪" as their primary stored key in the database, so I think weakening the validation is otherwise fine.

Actually, the biggest problem with loosening the validation is that my magic no-op priority is !!unknown!!, which becomes a valid priority under the "anything that isn't purely numeric" rule. I could change it to something like 11111, but that loses all meaning. I can't imagine the confusion of someone who came across that value in a debugger or stack trace.

Just change !!unknown!! to 900 consecutive goats and we're good to go.

And just to be clear, you're saying all three of these strings should be following the same validation rules, right?

  • priority keywords
  • status keywords
  • status names

Yeah -- or, slightly more broadly:

  • I think priority keywords and status keywords should follow the same validation rules.
  • If you want to let status names also have those looser validation rules too to keep things consistent I don't see any reason not to, we just need to change text12 to text64 (or whatever). There's also nothing really pushing this to happen, so we could leave it as-is for now too.

As compelling as having status == 🐫🐪 would be, I'm also envisioning issues with parsing email/bot commands if we start allowing !, quote characters, etc. I'm going to change the validation to 1-64 alphanumeric characters that aren't purely numeric and leave it at that.

Works for me.

amckinley updated this revision to Diff 43613.Jun 14 2017, 8:30 PM
  • change valiation, add migration

@epriestley can you sanity check my migration real quick before I land this?

epriestley added inline comments.Jun 14 2017, 9:25 PM
resources/sql/autopatches/20170614.taskstatus.sql
5

This should be COLLATE {$COLLATE_TEXT} -- on most systems that's utf8mb4_bin, but not always.

(Looks reasonable to me otherwise.)

amckinley updated this revision to Diff 43614.Jun 14 2017, 9:42 PM
  • correct collation
This revision was automatically updated to reflect the committed changes.
jcox added a subscriber: jcox.Jul 12 2017, 1:34 PM

This change appears to have made maniphest.edit (on the head of master) start failing on the following:

echo '{
  "transactions": [
    {
      "type": "priority",
      "value": "50"
    }
  ],
  "objectIdentifier": 12
}' | arc call-conduit maniphest.edit

With the following error:

Validation errors:
  - Task priority "50" is not a valid task priority. Use a priority keyword to choose a task priority: unbreak, triage, high, normal, low, wish.

Is this expected? I looked over this beforehand and expected it would be backwards compatible but it seems I was probably confused on where the conversation ended up.

Yes, expected. See 2017 Week 25 (Late June) for some specific guidance.

This behavior (accepting an integer, but only as a string) was so bizarre that it was essentially a bug, with "use an integer in quotes" as a workaround, kind of?