feat(jira-server): push a task to Jira Server integration

[igorlesnenko]

Resolves #5810 #5994 #6238

How to test:

• Add Jira Server integration
• Create a task in parabol
• Push task to Jira Server using push to integration menu
• See the task was created on Jira Server
• Add another member to a parabol team
• Login as a new member
• Create a task
• Login as a previous member
• Run the above query for the other member task
• See the task was created on Jira Server and a comment added
• Edit task in Jira Server
• Open parabol board, see the task is loaded correctly

[mattkrick]

-1 few little things, but nothing major. looking good!

[mattkrick]

I notice we use the same jira logo here, but a different jira logo for the team members integrations page.
From atlassian's logo page (https://atlassian.design/resources/logo-library), it looks like our jira server logo used on the integrations page may be outdated. @Dschoordsch should we update that to use the new version?

[igorlesnenko]

I feel like for tabs in poker meetings we use an older logo. I can replace it, but not sure which logo to use.

Here I just used the same logo, as expected there will be rarely both Jira and JiraServer integrations added.
However, in poker meeting tabs, 2 identical logos looks weird to me.

Not sure, if there is a separate server logo exists. Maybe add some s badge to the logo or something?

cc @Dschoordsch

[mattkrick]

This is the logo on our instance of Jira Server, whatcha think?

[igorlesnenko]

@mattkrick I see this square logo both on Jira Server and Jira in the header. However, I think we can use it for Jira Server to have different logos. Should we?

[Dschoordsch]

That's the logo for Jira Software, I think we should use it instead of the 3 arrows on the task card. Better version is available in the atlassian logo library. For the integration banner I had used the old logo they still use in their Jira Server documentation, could be the same Jira Software logo though.

[igorlesnenko]

Updated the logo

[mattkrick]

-1 it looks like this the TaskIntegrationManager is tightly coupled to the JiraServerRestManager.

I propose removing the complexity of the getManager() and instead just merging the 2:

export default class JiraServerTaskIntegrationManager
  extends TaskIntegrationManager, JiraServerRestManager

Note: extends vs. implements requires refactoring the TaskIntegrationManager to an abstract class. IMO abstract classes are cleaner than interfaces because it can lead to a single source of truth

[igorlesnenko]

@mattkrick As I see, class in TypeScript can't extend 2 classes.

I also think JiraServerTaskIntegrationManager and JiraServerRestManager have different purposes.

JiraServerTaskIntegrationManager exposes a unified API that is defined by abstract class or interface.
All Integration managers extend same abstract class and expose same API, for example taskIntegrationManager.createTask

This way avoid a lot of if constructions here

parabol/packages/server/graphql/mutations/createTaskIntegration.ts

Lines 125 to 131 in 5c97d33

	const createTaskResponse = await taskIntegrationManager.createTask({
	rawContentStr,
	integrationRepoId,
	createdBySomeoneElseComment,
	context,
	info
	})

However, internally, createTask relies on different createTask APIs — for GitHub it is GraphQL mutation, for JiraServer it is JiraServerRestManager etc.
If I simply extend, then there will be no unified shape of manager API, unified shape of input and output.

What do you think?

[mattkrick]

ahh, you're making a lot of sense. i'm all for making things non null wherever we can!
What if we invert the abstraction so we still only have 1 class that communicates with the service?

For example:

// Use an interface, just like your original, but no optional args on createTask!
interface TaskIntegrationManager {
  title: string

  createTask(params: {
    rawContentStr: string
    integrationRepoId: string
  }): Promise<CreateTaskResponse>
}

// The managers that talk to the service implement the TaskIntegrationManager
export default class JiraServerRestManager implements TaskIntegrationManager {
  constructor(auth, provider) {}
  createTask(str, repoId){}
}

// we grab the context, info in the constructor so methods like createTask don't need extra stuff
export default class GitHubServerManager implements TaskIntegrationManager {
  constructor(auth, provider, context, info) {}
  createTask(str, repoId){}
}

that way, our server managers can have all the wacky methods they want, but they'll still need to implement a few basic ones if they want to call themselves a TaskIntegrationManager

whatcha think?

[igorlesnenko]

@mattkrick Yes, I think we may merge two classes (JiraServerRestManager and JiraServerTaskIntegrationManager) into one.

But in this case, the resulting class would expose different methods that sound similar. For example: createIssue which is just a REST API call wrapper, and createTask — above mentioned unified method that wraps REST API method and might include additional actions.

Is it OK?

[mattkrick]

i think that's great!
if you wanna get fancy, the taskIntegrationFactory could return this Manager as a TaskIntegrationManager interface so those other methods will be hidden 😉

[igorlesnenko]

@mattkrick made some refactoring, can you take a look?

[igorlesnenko]

Made some fixes and little refactoring, including #6238

[mattkrick]

This is the logo on our instance of Jira Server, whatcha think?

[mattkrick]

i imagine this'll become non-null in the future, but happy to leave as is for now so we can get this big chunk merged master

[igorlesnenko]

@mattkrick it is already non-null in the interface, so removing this check in my next PR

[mattkrick]

wonderful stuff! the extra focus applied here to set the patterns should pay off when we repeat this pattern for the next integrations