Current JIRA integration is quite noisy in terms of email, and makes users hunt and peck for the related revisions.
Teach it to create an Issue Link on the JIRA side, and allow to disable commenting.
Details
Details
- Reviewers
eMxyzptlk epriestley btrahan avivey - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5422: Does Phabricator integrate with JIRA?
- Commits
- Restricted Diffusion Commit
rPe8fdf478bbc9: JIRA Integration: Link and/or Comment - Required Signatures
L28 Phacility Individual Contributor License Agreement
comment on revision in each of the 4 settings, check JIRA end for expected result.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- arcpatch-D9858
- Lint
Lint Warnings Severity Location Code Message Warning src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php:196 TXT3 Line Too Long Warning src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php:210 TXT3 Line Too Long Advice src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php:209 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 2022 Build 7584: [Placeholder Plan] Wait for 30 Seconds Build 2023: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
@epriestley what would you like to see in order to Accept this? It will help people using JIRA to adopt Phabricator Differential in their orgs. It has in mine.
Comment Actions
We're using this as a patch on our prod-phabricator, works perfect! please accept and land if possible.
Comment Actions
I wasn't aware of your implementation so I created my own with same purpose to have remote links: https://github.com/aik099/phabricator/commit/51d8d510d4113ddd89f6113afd03321b87f747f8
| src/applications/doorkeeper/option/PhabricatorJIRAConfigOptions.php | ||
|---|---|---|
| 3 | I think, that placing such configuration options directly on JIRA Auth Provider configuration screen would be more suitable because:
| |
| src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php | ||
| 200–201 | The DoorkeeperFeedWorkerJIRA operates on any object that have have PhabricatorEdgeConfig::TYPE_PHOB_HAS_JIRAISSUE edge linking it to external JIRA issue number. For that reason using:
might be more appropriate. | |
| 207 | Unfortunately this won't account for cases, when $object is of PhabricatorRepositoryCommit class. | |
| 213–215 | Nice catch. In my own implementation (that I created before I found yours) I haven't implemented this. | |
Comment Actions
Here is how link looks in my implementation:
I was trying to put link only on actual differential revision number, but it appeared too short to be usable.
Comment Actions
@epriestley can you please elaborate as to why this is classified as an unsupported patch as mentioned in T5422?
Comment Actions
@turadg can you please update this revision on top of master? I have already done so myself, I can Commandeer the revision and push my version if you'd like to.
Comment Actions
Thanks a lot @turadg. We are using this at Dailymotion and I promise I will keep it up to date (at least once a month). Please feel free to Commandeer it back if you'd like to make changes to it.
Comment Actions
- Merge branch 'master' into arcpatch-D9858
- getGroup is an abstract function of PhabricatorApplicationConfigOptions that must be implemented
- render a locally hosted icon 16x16
- Merge remote-tracking branch 'origin/master' into arcpatch-D9858
- Merge remote-tracking branch 'origin/master' into arcpatch-D9858
- fix lint issues.
Comment Actions
- Merge remote-tracking branch 'origin/master' into arcpatch-D9858
- Merge remote-tracking branch 'origin/master' into arcpatch-D9858
Comment Actions
- Replace all $object refs
- maybe add getMonogram($object) to Publisher
- "remote link type", "remote link name" ?
Comment Actions
"remote link type", "remote link name":
From the api link:
application.type is "name-spaced type of the application," and only used by custom renderers.
application.name is "human-readable name of the remote application instance`.
I think I'll go with @aik099's com.phacility.phabricator and Phabricator (Unless the "phacility" namespace is not right here?
Comment Actions
The link shows "[Differential] [Request, 2 lines]", which is kind annoying, but I don't want to change the Asana integration, etc. right now.
Maybe later we can separate the title from the extra information.
Comment Actions
I think introducing a getObjectFullName() would be fine, but we can do that later.
Couple of other inline suggestions but I didn't catch anything really substantive.
| src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php | ||
|---|---|---|
| 42 ↗ | (On Diff #34933) | If comments and links are disabled, maybe bail somewhere up here (before the external object query?) and log it ("All JIRA integration actions are disabled") to make debugging a little easier? |
| 198–200 ↗ | (On Diff #34933) | If we're creating a link, maybe we shouldn't add the URI? And/or, for JIRA, formatting this as:
...might make more sense, too. |
| 207 ↗ | (On Diff #34933) | Better would be celerity_get_resource_uri(). The base URI:
|
| 216 ↗ | (On Diff #34933) | Maybe just the PHID? The phabricatorPhid= part seems a little weird and PHID-DREV-... seems unlikely to collide with other application identifiers. If you want to retain this, consider spelling it as PHID over Phid for consistency. |
| 221 ↗ | (On Diff #34933) | I guess JIRA ain't big on internationalization. |
| src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php | ||
|---|---|---|
| 229 ↗ | (On Diff #34935) | Not sure if PHID is guaranteed to be unique across different Phabricator installs. In my implementation I've also added value of PhabricatorEnv::getEnvConfig('phabricator.base-uri') to the globalId. |
| src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php | ||
|---|---|---|
| 208–211 ↗ | (On Diff #34935) | looks like the logic is inverted here. if ($this->shouldPostLink()) {
return $text;
} else {
// include the link in the comment
return $text."\n\n".$publisher->getObjectURI($object);
} |
Comment Actions
(This is incorrect.)
I think the logic is correct -- the idea is that if the task is already linked at top level, we don't need to put a link in every comment (since it's redundant).
If we didn't create a top-level relationship, we do (there's no way to get to the object otherwise).
| src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php | ||
|---|---|---|
| 208–211 ↗ | (On Diff #34935) | Thanks - I knew it was a bad idea to send this last night :) |
| src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php | ||
|---|---|---|
| 229 ↗ | (On Diff #34935) |
@epriestley - what do you think about this? |
Comment Actions
PHIDs aren't guaranteed to be unique, but per the birthday paradox I think we expect one collision to exist globally once all installs produce a combined total of 1,125,000,000,000,000 revisions. We plan to kill JIRA before then.
Base URIs aren't hugely stable so I'd guess that adding one probably reduces the stability of the identifier for no real benefit.
Comment Actions
The base URI can also be arbitrarily long but the JIRA field is limited to 255 characters.