Page MenuHomePhabricator
Create Task
Maniphest T2968

In conduit, make auto resolve task via review land look the same as manual resolve
Open, NormalPublic
Actions

Description

They are slightly different, and this caused a bug in tenXer because we were expecting the string version of newValue. (I'm fixing that expectation in tenXer, but still better to be consistent because others might have the same issue.)

Example excerpts from https://tools.tenxer-corp.com/conduit/method/maniphest.gettasktransactions/

auto resolve

{
  "taskID"          : "5493",
  "transactionType" : "status",
  "oldValue"        : "0",
  "newValue"        : 1,
  "comments"        : null,
  "authorPHID"      : "PHID-USER-2d7perc36s5g6pd4dq7i",
  "dateCreated"     : "1365780938"
},

manual resolve

{
  "taskID"          : "5493",
  "transactionType" : "status",
  "oldValue"        : "0",
  "newValue"        : "1",
  "comments"        : "",
  "authorPHID"      : "PHID-USER-2d7perc36s5g6pd4dq7i",
  "dateCreated"     : "1365783330"
}

Related Objects
Search...

StatusAssignedTask
 DuplicateNoneT7869 Support CircleCI webhooks for Test results (so that one can run unit tests asynchronously)
 ResolvedepriestleyT9456 Evaluate upstream support for third-party build systems
 ResolvedepriestleyT9123 Build Phabricator in Harbormaster (v2)
 ResolvedepriestleyT2543 Add a formal "Draft" / "Not Yet Ready for Review" state to Differential
Restricted Maniphest Task
 ResolvedepriestleyT1315 Phacility Launch Status
 ResolvedepriestleyT2795 Phacility (Mid Priority)
 ResolvedchadT2637 Unbeta Pholio
 ResolvedchadT2636 It's difficult to know an inline comment didn't post/save in Pholio
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
 OpenNoneT3612 Lightbox v2
Restricted Maniphest Task
 OpenNoneT11332 Present Pholio inline comments in a lightbox
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
 DuplicateNoneT2644 Make Pholio work better on mobile
Restricted Maniphest Task
 ResolvedchadT1960 Breadcrumbs or similar UI-navigation for getting back to the ticket overview
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
 ResolvedNoneT1026 Request: ability to select line range for comment via touchscreen
Restricted Maniphest Task
Restricted Maniphest Task
 ResolvedhsbT1895 Disable live previews
Restricted Maniphest Task
Restricted Maniphest Task
 ResolvedepriestleyT4767 Autoclose and "Fixes/Ref" are too difficult for users to debug
 ResolvedepriestleyT5644 Add controls to prevent `pygmentize` from saturating system resources
 ResolvedepriestleyT5179 Unearth Differential rendering technical debt
 ResolvedjoshuaspenceT1751 Consider ways to improve revert handling
 ResolvedepriestleyT2542 Provide a link from revisions that are blamees to their blamers
 ResolvedbtrahanT3189 [feature request] Tag commits
Restricted Maniphest Task
 ResolvedbtrahanT5875 "Close Revision" and "Merge Task" interactions should be real transactions, not comments
Restricted Maniphest Task
 DuplicateepriestleyT5654 Show more context for inline comments in Differential timeline
 ResolvedepriestleyT2009 Refactor DifferentialChangesetParser to support 1-up views and sanity
Restricted Maniphest Task
 WontfixepriestleyT5742 Add commentor column to "Revision Update History" table
Restricted Maniphest Task
 ResolvedepriestleyT7358 When Replying to an Inline Comment, provide original context in email?
 ResolvedepriestleyT1460 Add "Done" link to an inline comment
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
 DuplicateNoneT3959 feed attributes all diff edits to diff owner
 ResolvedepriestleyT2222 Implement ApplicationTransactions in Differential
Restricted Maniphest Task
 ResolvedepriestleyT4264 Implement Repo/Project Herald rules to provide a middle ground between personal and global rules
 ResolvedepriestleyT5595 Allow all applications to filter results by project
 ResolvedepriestleyT2628 Add project tags to Pholio, Ponder, Differential, and other applications
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
 OpenNoneT2968 In conduit, make auto resolve task via review land look the same as manual resolve
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task

Event Timeline

cheryl added a subscriber: cheryl.Apr 12 2013, 6:16 PM
epriestley triaged this task as Normal priority.Apr 12 2013, 7:58 PM

I think there are two levels of systematic fix here:

  1. {T2217} should make transactions look consistent, at least (we can do a retroactive fix when we migrate, too).
  2. Ideally, we would be representing values properly in JSON (e.g., false would be better than either 0 or "0" as a parameter value) but this is a bit trickier because we can't store true and false in MySQL.

On (2), I intend to add typechecking to Conduit inputs at some point, and maybe we could improve the typing of output data when we do, too. This might have the effect of breaking you guys a bit, though. :/

The only practical way to deal with this right now might be to implement PHP rules for truthiness/falsiness, which makes all of "0", "", null, false and 0 falsey. Pretty blergh.

epriestley added a subscriber: epriestley.Apr 12 2013, 7:58 PM
epriestley edited this Maniphest Task.Apr 12 2013, 7:59 PM
cheryl added a comment.Apr 12 2013, 8:11 PM

Thanks for the response. Sounds like you are mostly describing fixes for boolean values but the newValue is not a boolean. Output data typing could help, and breaking our stuff...well, it happens. ;-) Only it's somewhat worse for Phabricator where people could be running different versions (vs say Github). This is fixed in tenXer now though (not deployed yet).

epriestley added a comment.Apr 12 2013, 8:14 PM

Ah, you're right. The bool stuff is an issue in general, but the "status" case here is just general type dubmness, as with nearly every other field: dateCreated should be an integer, taskID should be an integer, comments being empty should be represented consistently (not both as null and "").

rugabarbo added a project: Conduit.Apr 22 2014, 10:35 PM
rugabarbo added a subscriber: rugabarbo.
joshuaspence added a subscriber: joshuaspence.May 31 2015, 4:55 AM
chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 4:56 AM
eadler added a project: Restricted Project.Jan 9 2016, 1:06 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
rugabarbo removed a subscriber: rugabarbo.Feb 19 2016, 1:30 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:15 PM
Herald added a subscriber: eadler. · View Herald TranscriptJul 4 2016, 9:15 PM
eadler removed a project: Restricted Project.Jul 21 2016, 7:18 PM
eadler added a project: Restricted Project.Aug 5 2016, 5:24 PM
epriestley mentioned this in D18111: Use keywords instead of ints to update task priority in ManiphestEditEngine.Jun 12 2017, 7:00 PM
Phabricator · Built by Phacility