Page MenuHomePhabricator

JIRA Integration: Link and/or Comment
ClosedPublic

Authored by turadg on Jul 9 2014, 6:42 PM.
Referenced Files
Unknown Object (File)
Thu, Mar 28, 1:46 AM
Unknown Object (File)
Fri, Mar 22, 9:22 PM
Unknown Object (File)
Fri, Mar 22, 9:38 AM
Unknown Object (File)
Fri, Mar 22, 9:32 AM
Unknown Object (File)
Fri, Mar 22, 9:30 AM
Unknown Object (File)
Feb 27 2024, 1:01 AM
Unknown Object (File)
Feb 16 2024, 11:47 AM
Unknown Object (File)
Feb 15 2024, 8:07 PM
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
Branch
arcpatch-D9858
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 1637
Build 1638: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

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

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
215–216

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.

222

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

228–230

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

Here is how link looks in my implementation:

Jira_PhrabricatorIssueLink_AlexVersion.png (190×954 px, 48 KB)

I was trying to put link only on actual differential revision number, but it appeared too short to be usable.

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

Cool, Looking forward to trying this out.

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.

  • 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.
  • Merge remote-tracking branch 'origin/master' into arcpatch-D9858
  • Merge remote-tracking branch 'origin/master' into arcpatch-D9858
  • Merge branch 'master' into arcpatch-D9858
avivey added a reviewer: eMxyzptlk.

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.
  • Replace all $object refs
  • maybe add getMonogram($object) to Publisher
  • "remote link type", "remote link name" ?

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

pasted_file (109×601 px, 6 KB)

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.

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 ↗(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:

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

...might make more sense, too.

207 ↗(On Diff #34933)

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

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

celerity_get_resource_uri(), only link once, drop phabricatorPhid=

  • break out earlier
  • I'll run another test round tomorrow before landing
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.

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–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);
}
This revision now requires changes to proceed.Nov 10 2015, 5:14 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!

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)

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 edited edge metadata.
avivey marked an inline comment as done.
  • fix link text logic
  • Actually test this time
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 edited reviewers, added: avivey; removed: turadg.
This revision was automatically updated to reflect the committed changes.