Page MenuHomePhabricator

Allow the `todo` workflow to add project tags.
ClosedPublic

Authored by joshuaspence on Jun 10 2014, 6:05 PM.

Details

Summary

Fixes T4418. Allows Maniphests created through the arc todo workflow to have projects assigned.

Test Plan
> ./bin/arc --trace --conduit-uri='http://phabricator.joshuaspence.com' todo "Test project" --project foo --project bar
libphutil loaded from '/home/joshua/workspace/github.com/phacility/libphutil/src'.
arcanist loaded from '/home/joshua/workspace/github.com/phacility/arcanist/src'.
Config: Reading user configuration file "/home/joshua/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/home/joshua/workspace/github.com/phacility/arcanist/.arcconfig".
Working Copy: Path "/home/joshua/workspace/github.com/phacility/arcanist" is part of `git` working copy "/home/joshua/workspace/github.com/phacility/arcanist".
Working Copy: Project root is at "/home/joshua/workspace/github.com/phacility/arcanist".
Config: Did not find local configuration at "/home/joshua/workspace/github.com/phacility/arcanist/.git/arc/config".
Loading phutil library from '/home/joshua/workspace/github.com/phacility/arcanist/src'...
>>> [0] <conduit> conduit.connect() <bytes = 618>
>>> [1] <http> http://phabricator.joshuaspence.com/api/conduit.connect
<<< [1] <http> 1,050,487 us
<<< [0] <conduit> 1,051,585 us
>>> [2] <conduit> project.query() <bytes = 199>
>>> [3] <http> http://phabricator.joshuaspence.com/api/project.query
<<< [3] <http> 294,584 us
<<< [2] <conduit> 294,986 us
>>> [4] <conduit> maniphest.createtask() <bytes = 313>
>>> [5] <http> http://phabricator.joshuaspence.com/api/maniphest.createtask
<<< [5] <http> 637,693 us
<<< [4] <conduit> 638,098 us
Created task T6: 'Test project' at http://phabricator.joshuaspence.com/T6

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Allow the `todo` workflow to add project tags..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence edited edge metadata.
epriestley edited edge metadata.
epriestley added inline comments.
src/workflow/ArcanistTodoWorkflow.php
87–91

If the user mistypes a project (e.g., --project difusin) we should catch that and raise an error.

This is a little tricky because we can't map slugs to the resulting projects perfectly, but I think we could do this:

  • If we get fewer results back than we sent (unique) slugs, check slugs in more detail:
    • Resolve each slug individually and see if it hits a project.
    • If any did not hit a project, throw an error about their invalidity.

Since a user might specify two different slugs which are both valid and hit the same project (--project FOO --project fOo), it may not be an error even if we get fewer results back. Although maybe that should be a different error ("project X was specified twice"). But this seems OK to ignore, while typos seem important to catch.

This revision now requires changes to proceed.Jun 16 2014, 7:07 PM

Perhaps ConduitAPI_project_query_Method should raise an error if a specified slug doesn't exist?

joshuaspence edited edge metadata.

Throw a usage exception if an invalid slug is used.

While I think D9619 is good, I don't think this works. Particularly, if I type --project FOO, but the project really has slug #foo, the query will normalize the slug and find the project, but then this code will incorrectly determine that the slug wasn't found because the case is different. Is that accurate?

Elsewhere, we return a separate map (for example, the identifierMap on diffusion.querycommits). We could do this here, but it would break any existing callsites of projects.query since the top-level result format would change structure. Not the end of the world (we probably need to do this eventually to add pagination information) but seems like a lot of breaks for arc todo.

I don't think projects.query should fail on invalid slugs, either, since it's inconsistent with other methods and less useful than a method which returns partial results.

We could also return matchingSlugs on the results, but I don't particularly like the idea that the "object dictionary" we hand you had some non-object / query-dependent data. I don't think this is terrible, but it feels a bit surprising and is inconsistent with diffusion.querycommits.

src/workflow/ArcanistTodoWorkflow.php
96

array_merge() in a loop has really poor runtime behavior:

https://secure.phabricator.com/book/phabflavor/article/php_pitfalls/#array-merge-in-incredibl

Better is:

$project_slugs = array_mergev(ipull($projects, 'slugs'));

Best might be:

$project_slugs += array_fill_keys($project['slugs'], true);

...and then use isset() below (roughly, O(1)) instead of in_array() (roughly, O(N)).

While I think D9619 is good, I don't think this works. Particularly, if I type --project FOO, but the project really has slug #foo, the query will normalize the slug and find the project, but then this code will incorrectly determine that the slug wasn't found because the case is different. Is that accurate?

Are slugs always case-insensitive? We could pass them through strtolower?

I don't quite understand the issue. The query doesn't seem to normalize the slugs.

> echo '{"slugs": ["foo"]}' | arc --conduit-uri='http://phabricator.joshuaspence.com' call-conduit project.query
Waiting for JSON parameters on stdin...
{"error":null,"errorMessage":null,"response":{"PHID-PROJ-ttomlhslujpx5sdpbu2c":{"id":"1","phid":"PHID-PROJ-ttomlhslujpx5sdpbu2c","name":"Foo","members":["PHID-USER-cb5af6p4oepy5tlgqypi"],"slugs":{"1":"foo","2":"bar"},"dateCreated":"1402422720","dateModified":"1402422728"}}}

> echo '{"slugs": ["FOO"]}' | arc --conduit-uri='http://phabricator.joshuaspence.com' call-conduit project.query
Waiting for JSON parameters on stdin...
{"error":null,"errorMessage":null,"response":[]}

Oh -- well, it should be normalizing -- that is, --project Diffusion, --project 'Quality Assurance', etc., should work.

One approach would be to introduce project.normalizeslugs or something?

Oh -- well, it should be normalizing -- that is, --project Diffusion, --project 'Quality Assurance', etc., should work.

One approach would be to introduce project.normalizeslugs or something?

That means we'd need to do two Conduit calls... first to normalize the slugs and then to query the project. What about introducing project.queryslugs instead?

Oh -- well, it should be normalizing -- that is, --project Diffusion, --project 'Quality Assurance', etc., should work.

One approach would be to introduce project.normalizeslugs or something?

That means we'd need to do two Conduit calls... first to normalize the slugs and then to query the project. What about introducing project.queryslugs instead?

@epriestley, how do you think that I should tackle this problem?

We don't actually call project.query anywhere, so let's just modernize it to work like the most modern methods (e.g., diffusion.querycommits), and return:

  • a list of results in data (the current results of the entire call);
  • paging parameters; and
  • a slugMap mapping any queried slugs to their associated projects PHIDs.

We need to do this sooner or later anyway.

joshuaspence edited edge metadata.
  • Improved efficiency

Just pinging you on this which should be okay (I think) after D9619.

I think this should use slugMap so it continues working after we start normalizing slugs?

epriestley edited edge metadata.
This revision is now accepted and ready to land.Jul 10 2014, 11:42 AM