Details

Reviewers

0
• avivey
valerio.bozzolan

Group Reviewers

Blessed Committers
O1: Blessed Committers

Maniphest Tasks

T15484: Conduit project.column.search: missing isHidden, isDefaultColumn, sequence
T15081: Figure out if there are patches from Wikimedia's fork that are desirable to upstream in Phorge

Commits

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

Summary

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

Knowing:

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

Branch

phorge.it (branched from master)

Lint

Lint Warnings

Severity	Location	Code	Message
Warning	src/applications/project/storage/PhabricatorProjectColumn.php:285	TXT3	Line Too Long

Unit

Test Failures

Build Status

Buildable 67
Build 67: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	115 ms	PhabricatorCelerityTestCase::testCelerityMaps Assertion failed, expected 'true' (at PhabricatorCelerityTestCase.php:32): When this test fails, it means the Celerity resource map is out of date. Run `bin/celerity map` to rebuild it. ACTUAL VALUE
	13 ms	PhabricatorConduitTestCase::testConduitMethods 1 assertion passed.
	0 ms	PhabricatorInfrastructureTestCase::testApplicationsInstalled 1 assertion passed.
	0 ms	PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries 1 assertion passed.
	433 ms	PhabricatorLibraryTestCase::testEverythingImplemented 1 assertion passed.
		View Full Test Results (1 Failed · 29 Passed)

Event Timeline

20after4 created this revision.Apr 24 2022, 18:29

Owners added a reviewer: O1: Blessed Committers.Apr 24 2022, 18:29

Herald added subscribers: Matthew, valerio.bozzolan, tobiaswiese, speck. · View Herald TranscriptApr 24 2022, 18:29

20after4 requested review of this revision.Apr 24 2022, 18:29

Harbormaster completed remote builds in B67: Diff 93.Apr 24 2022, 18:29

celerity map

Harbormaster completed remote builds in B68: Diff 94.Apr 24 2022, 18:30

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)

Fix line length

Harbormaster completed remote builds in B69: Diff 95.Apr 24 2022, 18:36

20after4 mentioned this in T15081: Figure out if there are patches from Wikimedia's fork that are desirable to upstream in Phorge.Apr 24 2022, 18:39

20after4 added a task: T15081: Figure out if there are patches from Wikimedia's fork that are desirable to upstream in Phorge.

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

Cigaryno subscribed.Sep 8 2022, 13:16

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

• avivey commandeered this revision.Mar 16 2023, 19:23

• avivey added a reviewer: 20after4.

change language and replace "status" with "isHidden"
celerify

Harbormaster completed remote builds in B135: Diff 344.Mar 16 2023, 19:37

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

0 added inline comments.Mar 17 2023, 04:17

src/applications/project/storage/PhabricatorProjectColumn.php
294	Should the key be changed to match?

• avivey added inline comments.Mar 17 2023, 07:18

src/applications/project/storage/PhabricatorProjectColumn.php
294	Yes, they should :)

Harbormaster completed remote builds in B136: Diff 345.Mar 17 2023, 07:19

0 accepted this revision.Mar 18 2023, 05:43

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.

valerio.bozzolan added inline comments.Mar 27 2023, 13:53

src/applications/project/storage/PhabricatorProjectColumn.php
296	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)

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

valerio.bozzolan foisted this revision upon 20after4.Apr 7 2023, 22:01

valerio.bozzolan edited reviewers, added: valerio.bozzolan; removed: 20after4.

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.May 18 2023, 10:16

valerio.bozzolan added inline comments.Jun 6 2023, 09:18

src/applications/project/storage/PhabricatorProjectColumn.php
296	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

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

src/applications/project/storage/PhabricatorProjectColumn.php
296	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.