Allow moving integration tasks between teams

[Dschoordsch]

If a task with the same hash exists in the destination, it is deleted first.

A conflicting integration task in the target is deleted first and the delete notifications are sent out, then the original task is moved.
In order to avoid having duplicated functionality the teamId was removed from updateTask mutation, so changeTaskTeam is the only way left to move a task.

Testing

Moving task between teams replacing existing task

Setup

• create 2 teams Team A and Team B
• join both teams with users Alice and Bob
• Alice sets up Jira integration for Team A and Team B
• Bob sets up Jira integration for Team B only
• Alice starts a Sprint Poker in Team A and adds 4 tasks in the Scope stage via Jira integration (JIRA-1 - JIRA-4)
• Bob does the same in Team B with the same 4 Jira tasks
• Alice goes to their Tasks, Archived and un-archive the Jira tasks and distributes them in all 4 columns
• Bob goes to their Tasks, Archived and un-archives JIRA-1 - JIRA-3 and leaves them in the default column
• Open the sessions for Alice and Bob in separate browsers

Verification

• Bob opens team B dashboard
• Alice opens Tasks and changes the team of JIRA-1 task from Team A to Team B
• While Alice is doing this, Bob observes the old Team B JIRA-1 disappearing and the new Team A JIRA-1 appearing
• Repeat for JIRA-2 till JIRA-4, the tasks must appear in the right column for Bob
• The archived JIRA-4 of Bob is gone

Moving task to new team without integration

Setup

• Clarissa creates 2 teams: Team Integration and Team No-Integration
• Clarissa adds GitHub and Jira integrations to Team Integration
• Clarissa creates a new GitHub and a new Jira Task

Verification

• in Task view Clarissa selects Team No-Integration for the GitHub Task from the team drop down
  • a dialog is presented asking Clarissa to setup GitHub integration
• Clarissa declines
  • task is still in Team No-Integration
• Clarissa repeats the process but now presses 'Add it now'
  • GitHub oauth is started
  • when successfull task is moved to Team Integration
• Repeat verification steps with Jira

Fixes: #5200

[Dschoordsch]

@nickoferrall could you take a look please?

[nickoferrall]

Hey @Dschoordsch, would you mind clarifying a few things in the testing steps please:

1. Is the second client using the same user account as the first client or are they two different users?
2. By integration tasks, are you referring to regular Parabol tasks added in Sprint Poker, or tasks that are integrated with Jira or GitHub?
3. Does target dashboard refer to the /tasks page?
4. Should I create two Sprint Poker meetings, one for each team, and add the identical task in both meetings?

If you wouldn't mind adding a bit more detail to the testing steps, I'll review the PR asap!

[Dschoordsch]

@nickoferrall I updated the testing steps to be more specific, let me know if that works for you.

[nickoferrall]

Thanks for updating the testing steps, that's very helpful! I've run through each step and while the Team B task is disappearing and the Team A task is appearing, the content of the task changes to Task imported from jira for me. This happens for both User Alice & Bob:

[Dschoordsch]

@nickoferrall Awesome find! In my test Bob had the integration set up for both Team A and Team B, then it works. Apparently we're using the integration of the assignee and team combination.
I would argue fixing this is out of scope for this ticket but should be part of AC for #5342

[nickoferrall]

Creating a new issue makes sense to me. Great work finding a fix for such a tricky edge case! LGTM

[Dschoordsch]

@nickoferrall I made additional changes to catch the use case when a user tries to move a task to a team without having the integration set up for it yet. I also added separate testing flow for it. Could you please review again?

[nickoferrall]

That's all looking good to me! Feel free to review my comments and then proceed to Maintainer Review

[nickoferrall]

I find the text here a little bit hard to read. Perhaps fontSize 14 with lineHeight 20?

[Dschoordsch]

Yes, looks much better. It came from AddMissingJiraFieldModal so we might want to increase size there as well

[Dschoordsch]

@enriquesanchez I added a dialog which appears when a user tries to move a task with integration to a team where they didn't set an integration up yet, is this dialog ok:

?

[Dschoordsch]

@mattkrick could you please review?

[enriquesanchez]

Hi @Dschoordsch

What happens if the user clicks cancel? Would the task still be moved and disconnected from the integration? Or would it abort the action altogether?

Design wise looks good! My only suggestion is to use the main Sky blue color for the Add it now button instead of the red/orange gradient. We only use that color gradient in buttons in very specific actions (like creating a new meeting), so I think it's best to stick to the main blue for this specific case.

[mattkrick]

agreed the blue does feel better, although our prior art is to use the primary CTA for these modals. we'll probably need to revisit all the other modals like this & change the color there, too (removing a team member, removing an org user, asking for super permission from jira to fix their integration, promoting to team lead, etc....)

[enriquesanchez]

agreed the blue does feel better, although our prior art is to use the primary CTA for these modals

Thanks @mattkrick 🙌

@Dschoordsch I think then it's best to keep it as is right now, and further down the line I can look at updating these colors altogether.

[Dschoordsch]

What happens if the user clicks cancel? Would the task still be moved and disconnected from the integration? Or would it abort the action altogether?

The action is aborted.

[mattkrick]

i think the execution here is good, my only question is would anyone in real life click the cancel button? if not, maybe we don't even prompt them & we just do it behind the scenes for them.

[Dschoordsch]

I can imagine that people might reconsider adding the integration to the target team, but for me it's more important that they are informed that the new integration is added to the team. To me moving over the integration to the new team is not obvious, if I think about it, then it is logical, but when I'm moving a task I'm not thinking about what the tool might do in the background.

[mattkrick]

Loom: https://www.loom.com/share/22327acd2f8643718f31cad2e572ec52

[mattkrick]

it's possible that someone calls this mutation outside of our UI.
If that happens, we need this mutation to guarantee that an integration exists on the new teamId

I propose the following:

• remove the OAuth2 flow from the client, if they click cancel, we abort, just like you have written. if they click OK, then we call this mutation
• this mutation looks up the AtlassianAuth (or GitHubAuth) for the viewerId/teamId combination
• if it exists, the teamId param is valid
• if it doesn't exist, then it looks up the Auth for the viewerId/oldTeamId
• if that doesn't exist, return error: No valid integration auth
• If that does exist, then copy that row & insert it with the updated teamId

eliminating the extra oauth2 flow means the process goes faster & more importantly we can verify that the new teamId supports that task integration

[Dschoordsch]

I got a bit lost in edge cases. When the viewer has no integration in the target team, there are these options:

1. if user has integration in source team, then we will use that, but still ask
2. if accessUser is someone else and they have integration for target team, then we will use that without asking
3. else we need to ask the user to integrate with complete oauth flow

For now I ignore 2) and 3) and just assume it's 1) as that's the most common case in the UI.
Better would be of course to detect 2) and 3) as well and handle it appropriately in the UI, but we do not get all information necessary at the moment via GraphQL and I also don't want to repeat the same logic if there is a nicer solution.

Let's just roll with 1) and fix 2) and 3) another day, user value is probably also minimal. Use case 2) I can imagine is a bit annoying, i.e. team lead has 2 parabol teams and adds integration. They're also the only ones with access to Jira. Every time some other team member would move a task, they would get shown the dialog. It would still move the task, but it might get annoying. But if that hypothetical user exists, they can open a ticket.

[mattkrick]

fantastic work exploring all the edge cases.
i think your assumption are reasonable & i feel very comfortable with the solution you've built!

[mattkrick]

we should probably try to use task.integration?.accessUserId too

[Dschoordsch]

true

[mattkrick]

the && will turn into a false.
currently in the DB, the integration field doesn't exist on a Task if it is not integrated.
to keep this same structure, we would want this to be undefined if there is no integration or if the integration is unchanged (rethinkdb driver filters out keys with undefined values). Gotta love NoSQL 😞

For your consideration, I wrapped all the integration stuff up in a single conditional. This might be useful as we move forward with jira server & gitlab integrations. Then you can just write integration: updatedIntegration:

    const {integration} = task
    let updatedIntegration: typeof integration
    if (integration) {
      const {accessUserId, service} = integration
      const authKeys = [
        {teamId: task.teamId, userId: viewerId},
        {teamId, userId: viewerId},
        {teamId, userId: accessUserId}
      ]
      const errToNullMap = <T>(val: T | Error | null) => (val instanceof Error ? null : val)
      const [sourceTeamAuth, targetTeamAuth, accessUsersTargetTeamAuth] =
        service === 'jira'
          ? (await dataLoader.get('freshAtlassianAuth').loadMany(authKeys)).map(errToNullMap)
          : service === 'github'
          ? (await dataLoader.get('githubAuth').loadMany(authKeys)).map(errToNullMap)
          : []
      if (!targetTeamAuth && !sourceTeamAuth && !accessUsersTargetTeamAuth) {
        return standardError(new Error('No valid integration found'), {
          userId: viewerId
        })
      }
      const data = {teamId, userId: viewerId}
      // use viewer's auth or fallback to the existing accessUserId
      const nextAccessUserId = sourceTeamAuth || targetTeamAuth ? viewerId : accessUserId
      if (nextAccessUserId !== accessUserId) {
        updatedIntegration = {...integration, accessUserId: nextAccessUserId}
      }
      if (sourceTeamAuth && !targetTeamAuth) {
        if (service === 'jira') {
          await upsertAtlassianAuth({
            ...sourceTeamAuth,
            teamId
          })
          publish(SubscriptionChannel.TEAM, teamId, 'AddAtlassianAuthPayload', data, subOptions)
        } else if (service === 'github') {
          await upsertGitHubAuth({
            ...sourceTeamAuth,
            teamId
          })
          publish(SubscriptionChannel.TEAM, teamId, 'AddGitHubAuthPayload', data, subOptions)
        }
      }
    }

[Dschoordsch]

While I like the updatedIntegration cleanup, the current && would still work, since it would return undefined when task.integration === undefined. && doesn't convert the result to boolean.

[mattkrick]

ah you're right! that's what i get for thinking at night

[mattkrick]

these might be good candidates for loadMany, although those have the possibility of returning an Error instance.
I wrote an example of what that might look like, let me know what you think

[Dschoordsch]

I don't really like the loadMany signature and it's just a wrapper for Promise.all(load(...)). Question is whether we want to silently ignore errors and convert them to null or let them bubble up. I tend to default to just let these bubble up since we don't expect any errors here. But I can reduce repetition.

[mattkrick]

agreed, the signature used to be so much nicer! graphql/dataloader#216
we're already silently ignoring any errors inside the batchFn, i wish we could tell the dataloader that we don't need any errors that come out of it.

mapping & wrapping in Promise.all is probably the cleanest way as long as we can guarantee that none of them will throw.

[mattkrick]

fantastic work exploring all the edge cases.
i think your assumption are reasonable & i feel very comfortable with the solution you've built!