Page MenuHomePhabricator

JIRA Integration: Link and/or Comment
ClosedPublic

Authored by turadg on Jul 9 2014, 6:42 PM.
Subscribers
Tokens
"Mountain of Wealth" token, awarded by jra3."Like" token, awarded by rmuslimov."Love" token, awarded by waynea."Like" token, awarded by hach-que.

Details

Summary

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.

Test Plan

comment on revision in each of the 4 settings, check JIRA end for expected result.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
turadg added a comment.Aug 3 2014, 9:11 PM
  • conform to most linting
waynea awarded a token.Aug 6 2014, 4:40 AM
waynea added a subscriber: waynea.
turadg retitled this revision from Link on JIRA ticket instead of commenting to Link on JIRA ticket (optionally).Aug 8 2014, 10:22 PM
turadg updated this object.
turadg added a comment.Sep 9 2014, 3:59 PM

@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.

rmuslimov added a subscriber: rmuslimov.

We're using this as a patch on our prod-phabricator, works perfect! please accept and land if possible.

svemir added a subscriber: svemir.Oct 17 2014, 12:38 AM
aik099 added a subscriber: aik099.Jan 2 2015, 9:38 PM

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 ↗(On Diff #24377)

I think, that placing such configuration options directly on JIRA Auth Provider configuration screen would be more suitable because:

  • they don't apply to all Auth Providers
  • it's better to have them configured per auth provider because several Jira instances might be connected and each might have different rules
src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php
200–201 ↗(On Diff #24377)

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:

  • com.phacility.phabricator as remote link type
  • Phabricator as remote link name

might be more appropriate.

207 ↗(On Diff #24377)

Unfortunately this won't account for cases, when $object is of PhabricatorRepositoryCommit class.

213–215 ↗(On Diff #24377)

Nice catch. In my own implementation (that I created before I found yours) I haven't implemented this.

aik099 added a comment.Jan 2 2015, 9:54 PM

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.

frenchs added a subscriber: frenchs.Apr 6 2015, 4:40 PM

@epriestley can you please elaborate as to why this is classified as an unsupported patch as mentioned in T5422?

@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.

turadg added a comment.Jul 7 2015, 7:11 PM

@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.

Thanks for offering. Go for it @eMxyzptlk.

jra3 added a subscriber: jra3.Jul 7 2015, 10:23 PM

Cool, Looking forward to trying this out.

eMxyzptlk commandeered this revision.Jul 14 2015, 10:07 PM
eMxyzptlk added a reviewer: turadg.

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.

eMxyzptlk updated this revision to Diff 32944.Jul 14 2015, 10:20 PM
  • 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.
eMxyzptlk updated this revision to Diff 33515.Aug 12 2015, 10:10 PM
  • Merge remote-tracking branch 'origin/master' into arcpatch-D9858
  • Merge remote-tracking branch 'origin/master' into arcpatch-D9858
eMxyzptlk updated this revision to Diff 34093.Sep 14 2015, 8:44 PM
  • Merge branch 'master' into arcpatch-D9858
jra3 awarded a token.Sep 14 2015, 9:02 PM
vhbit added a subscriber: vhbit.Oct 8 2015, 2:45 PM
avivey added a subscriber: avivey.Nov 6 2015, 7:20 PM
avivey commandeered this revision.Nov 8 2015, 2:12 AM
avivey added a reviewer: eMxyzptlk.
avivey updated this revision to Diff 34885.Nov 8 2015, 2:13 AM

move configuration to provider.

Still hadn't handled some of the feedback.

avivey retitled this revision from Link on JIRA ticket (optionally) to JIRA Integration: Link and/or Comment.Nov 8 2015, 2:16 AM
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a project: Doorkeeper.
avivey planned changes to this revision.Nov 10 2015, 12:07 AM
  • Replace all $object refs
  • maybe add getMonogram($object) to Publisher
  • "remote link type", "remote link name" ?
avivey added a comment.EditedNov 10 2015, 12:22 AM

"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?

avivey updated this revision to Diff 34933.Nov 10 2015, 1:09 AM

Basically copy what @aik099 did.

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.

avivey edited the test plan for this revision. (Show Details)Nov 10 2015, 1:11 AM
epriestley accepted this revision.Nov 10 2015, 1:21 AM
epriestley edited edge metadata.

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–50

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?

206–213

If we're creating a link, maybe we shouldn't add the URI?

And/or, for JIRA, formatting this as:

In D123 https://.../:
body body body

...might make more sense, too.

220

Better would be celerity_get_resource_uri(). The base URI:

  • may be a development/test host;
  • may not be the correct CDN domain;
  • won't version correctly if the underlying resource is updated.
229

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.

234

I guess JIRA ain't big on internationalization.

This revision is now accepted and ready to land.Nov 10 2015, 1:21 AM
avivey updated this revision to Diff 34934.Nov 10 2015, 3:34 AM
avivey marked 4 inline comments as done.
avivey edited edge metadata.

celerity_get_resource_uri(), only link once, drop phabricatorPhid=

avivey updated this revision to Diff 34935.Nov 10 2015, 3:36 AM
  • break out earlier
  • I'll run another test round tomorrow before landing
aik099 added inline comments.Nov 10 2015, 8:15 AM
src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php
229

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.

turadg requested changes to this revision.Nov 10 2015, 5:14 PM
turadg edited edge metadata.
turadg added inline comments.
src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php
208–214

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);
}
This revision now requires changes to proceed.Nov 10 2015, 5:14 PM
epriestley added a comment.EditedNov 10 2015, 5:40 PM

(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).

Oh, sorry, you're right. I read your inline instead of the code. Never mind!

avivey added inline comments.Nov 10 2015, 6:11 PM
src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php
208–214

Thanks - I knew it was a bad idea to send this last night :)

avivey added inline comments.Nov 10 2015, 6:32 PM
src/applications/doorkeeper/worker/DoorkeeperJIRAFeedWorker.php
229

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.

@epriestley - what do you think about this?

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.

The base URI can also be arbitrarily long but the JIRA field is limited to 255 characters.

avivey updated this revision to Diff 34980.EditedNov 11 2015, 6:58 PM
avivey edited edge metadata.
avivey marked an inline comment as done.
  • fix link text logic
  • Actually test this time
turadg accepted this revision.Nov 11 2015, 11:17 PM
turadg edited edge metadata.

@avivey thanks for bringing this up to snuff for merging upstream!

This revision is now accepted and ready to land.Nov 11 2015, 11:17 PM
turadg commandeered this revision.Nov 12 2015, 7:30 PM
turadg edited reviewers, added: avivey; removed: turadg.
This revision was automatically updated to reflect the committed changes.