Page MenuHomePhorge

Add Status, sequence and isDefault to the conduit api results for column.search
Needs ReviewPublic

Authored by avivey on Apr 24 2022, 18:29.
Tags
None
Referenced Files
F270897: D25038.id344.diff
Fri, Mar 24, 13:09
F270852: D25038.id345.diff
Fri, Mar 24, 06:43
Unknown Object (File)
Tue, Mar 21, 21:51
Unknown Object (File)
Tue, Mar 21, 07:33
Unknown Object (File)
Mon, Mar 20, 20:20
Unknown Object (File)
Fri, Mar 17, 02:05
Unknown Object (File)
Sun, Mar 12, 23:24
Unknown Object (File)
Tue, Mar 7, 18:22

Details

Summary

This seems like a fairly obvious oversight with the column.search api.

Knowing

  1. the status (active vs archived)
  2. isDefaultColumn (the one that tasks get dropped in by default)
  3. sequence (order on the workboard)

are all necessary for a lot of things that very sensible real-world api clients need to do when working with columns.

Cherry-picked from https://phabricator.wikimedia.org/rPHABebfe30890b52784d222ec4ed36c05462b2a57f92

Test Plan

Tested on phabricator.wikimedia.org over many months and used by real client apps.

Diff Detail

Repository
rP Phorge
Branch
phorge.it (branched from master)
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningsrc/applications/project/storage/PhabricatorProjectColumn.php:285TXT3Line Too Long
Unit
Tests Passed
Build Status
Buildable 68
Build 68: arc lint + arc unit

Event Timeline

20after4 retitled this revision from Add column sequence to the conduit api results for column.search to Add Status, sequence and isDefault to the conduit api results for column.search.Apr 24 2022, 18:34
20after4 edited the summary of this revision. (Show Details)
0 requested changes to this revision.May 22 2022, 19:22
0 added a subscriber: 0.
0 added inline comments.
src/applications/project/storage/PhabricatorProjectColumn.php
276

It seems odd to return the strings "0" and "1" for the status. Unless there's a reason to do this, an integer would be more reasonable. Even better would be an array containing the keys "value" and "name", like in a few other status fields.

281

The existing bool fields use the language "True if ..." rather than "Whether ...".

This revision now requires changes to proceed.May 22 2022, 19:22

Thank you @20after4 for this patch. The inline comments seem reasonable to me. What do you think about?

avivey added a reviewer: 20after4.
  • change language and replace "status" with "isHidden"
  • celerify

(I haven't seen @20after4 for a while now. Also @0).

src/applications/project/storage/PhabricatorProjectColumn.php
294

Should the key be changed to match?

src/applications/project/storage/PhabricatorProjectColumn.php
294

Yes, they should :)

I tested this locally and it works, thank you.

The only strange thing is that the sequence field is supposed - internally - to be an integer but it's exposed as a string in this API.

I say, that it's supposed to be an int, since I noticed:

PhabricatorBoardLayoutEngine.php
$next_sequence = last($board_columns)->getSequence() + 1;

And:

PhabricatorProjectColumnPosition.php
->addInt($this->getSequence())

So, I suggest to expose the sequence field as an integer, and not as a string.

In case, I suggest to return an int just from the API, without touching in any way the PhabricatorEdgeObject#getSequence() - in this moment.