Page MenuHomePhorge

Conduit column.search: add status, sequence and isDefault to API results
ClosedPublic

Authored by 20after4 on Apr 24 2022, 18:29.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 20, 21:40
Unknown Object (File)
Mon, May 20, 11:14
Unknown Object (File)
Fri, May 17, 19:19
Unknown Object (File)
Fri, May 17, 18:51
Unknown Object (File)
Fri, May 17, 16:30
Unknown Object (File)
Fri, May 17, 15:55
Unknown Object (File)
Fri, May 17, 14:44
Unknown Object (File)
Sun, May 12, 01:45

Details

Summary

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

Knowing:

  1. isHidden - boolean to indicate Active vs Archived
  2. isDefaultColumn - the one that Tasks get dropped in by default, usually called "Backlog"
  3. sequence - numerical 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.

Partial cherry-pick from:

https://phabricator.wikimedia.org/rPHABebfe30890b52784d222ec4ed36c05462b2a57f92

Closes T15484

Test Plan

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

To do additional tests, visit this page:

/conduit/method/project.column.search/

Check that the new fields are returned correctly and nothing explodes.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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 subscribed.
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
295

Should the key be changed to match?

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

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.

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

As already mentioned I would recommend to return an integer here since all the known business logic of that information assume that as an integer, and so the API consumers should assume that as an integer.

I'm not opinionated about how, but maybe a single cast can do the job instead of doing a refactor, for now.

valerio.bozzolan retitled this revision from Add Status, sequence and isDefault to the conduit api results for column.search to Conduit column.search: add status, sequence and isDefault to API results.Apr 7 2023, 21:58
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited reviewers, added: avivey; removed: valerio.bozzolan.

@20after4 do you agree in exposing the sequence as an integer? (since it's assumed to be an integer internally)

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

I'm suggesting a cast to int also because there are other places related to Conduit with a cast. Example:

rPef13b0e52b46: Expose repository "importing" flag via diffusion.repository.search

0 requested changes to this revision.Jun 8 2023, 04:49

@valerio.bozzolan, do you want to commandeer this revision and add the cast yourself?

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

The sequence should always be an integer, so adding the cast here seems reasonable. As this sort of thing is already being done in several places, perhaps the comment isn't necessary. (Another example is (int)$this->getVersion() for phriction.content.search.)

Although it is odd that these methods are returning string values in the first place. This is too deep in the bowels of Phabricator for me to understand, but I don't think PhabricatorEdgeObject#getSequence() is to blame; it might have something to do with readField in LiskDAO instead. I bailed out when I saw "Black magic" in one of the comments.

This revision now requires changes to proceed.Jun 8 2023, 04:49

expose sequence as an integer

I have a hard time imagining a situation where this is not helpful to other people. Thanks again 20after4. I've just casted a thing to int.

Thanks again! Hoping to be useful I will land this in 4 days :)

This revision is now accepted and ready to land.Jun 14 2023, 00:29