Page MenuHomePhabricator

First stab at a conduit method for creating projects.
ClosedPublic

Authored by 20after4 on Jul 24 2014, 8:47 AM.

Details

Summary

This code is mostly lifted from the PhabricatorProjectCreateController.

Test Plan

currently untested

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

20after4 added a task: Restricted Maniphest Task.
20after4 retitled this revision from to First stab at a conduit method for creating projects..
20after4 edited the test plan for this revision. (Show Details)
20after4 added reviewers: chasemp, epriestley.
20after4 added a subscriber: aklapper.
20after4 edited edge metadata.
epriestley edited edge metadata.

This is basically fine. Some general inlines.

src/applications/project/conduit/ConduitAPI_project_create_Method.php
3–5 ↗(On Diff #24123)

You can drop these @group comments now (I think we removed them all recently from the rest of the codebase). They're set by config instead, now.

6 ↗(On Diff #24123)

We're about to rename all the Conduit stuff to be more consistent, see D9991.

8 ↗(On Diff #24123)

After D9991, you'll need to explicitly specify that the method name is 'project.create'.

9 ↗(On Diff #24123)

pht()

24–26 ↗(On Diff #24123)

Just drop this, I want to move Conduit more toward handling exceptions in a reasonable way rather than defining a custom exception type for each exception. In practice, no one cares about or handles these special cases, and we can implement them more simply/generically at a higher level anyway.

33–34 ↗(On Diff #24123)

ConduitMethod might not have this method, you can add it if that's the case.

38–48 ↗(On Diff #24123)

This can be simplified a bit, just ->setNewValue($request->getValue('name')) without lines 38..44

54 ↗(On Diff #24123)

This should be unnecessary.

56–62 ↗(On Diff #24123)

Just do:

'+' => array_fuse($members)

...instead of looping. That is, generate one transaction which sets all of the members.

68 ↗(On Diff #24123)

I think this only accepts AphrontRequest, not ConduitRequest.

69–74 ↗(On Diff #24123)

Just let this exception escape.

This revision now requires changes to proceed.Jul 24 2014, 8:59 PM
20after4 edited edge metadata.
  • Rebased on top of the changes from D9991
  • Addressed @epriestley's feedback
  • implemented ConduitAPIMethod::requireApplicationCapability
epriestley edited edge metadata.

A little minor cleanup, one behavioral issue. This looks good otherwise.

src/applications/project/conduit/ProjectCreateConduitAPIMethod.php
4–6

Nuke

14

Wrap this in pht() so it can be translated.

42

Unused.

44

Unused.

52

Adding the user is a surprising behavior to me for an API method. Do we need this for something to work? If so, maybe we should fix that. If not, let's just omit it?

This is consistent with how the web workflow works, but it feels odd to me in an API context. I think it's reasonable to expect API callers to include themselves if they want to be project members, and also reasonable that bot/script project creation may not want to make projectbot a member of every project.

This revision now requires changes to proceed.Aug 5 2014, 3:06 PM
20after4 edited edge metadata.
  • Correct issues identified by @epriestley's review
  • Simplified slightly
epriestley edited edge metadata.
This revision is now accepted and ready to land.Aug 5 2014, 4:27 PM
epriestley updated this revision to Diff 24421.

Closed by commit rP5f82705e1234 (authored by @20after4, committed by @epriestley).