Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow moving integration tasks between teams #5513

Merged
merged 15 commits into from Nov 11, 2021
Merged

Allow moving integration tasks between teams #5513

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
81ef6b2
Allow moving integration tasks between teams
Dschoordsch Oct 13, 2021
0847523
Remove teamId from updateTask mutation
Dschoordsch Oct 13, 2021
9b28d90
Merge branch 'master' into feat/5200/allow-moving-teams-of-integratio…
Dschoordsch Nov 1, 2021
e5f7267
Before moving a task to another team check if an integration is needed
Dschoordsch Nov 1, 2021
952ee95
Fix TaskFooterTeamAssigneeMenu_task fragment
Dschoordsch Nov 1, 2021
10b24cf
Show add integration dialog when moving task to team w/o integration
Dschoordsch Nov 3, 2021
6973960
Polish add integration now dialog
Dschoordsch Nov 5, 2021
7921814
Merge branch 'master' into feat/5200/allow-moving-teams-of-integratio…
mattkrick Nov 5, 2021
5fa6716
Merge branch 'master' into feat/5200/allow-moving-teams-of-integratio…
mattkrick Nov 5, 2021
cc7d172
Cleanup
Dschoordsch Nov 8, 2021
aaa72be
Copy integration first before moving a task to a different team
Dschoordsch Nov 9, 2021
5266696
Don't start OAuth flow when moving a to another team w/o integration
Dschoordsch Nov 9, 2021
6cf7590
Cleanup
Dschoordsch Nov 9, 2021
be84ae2
Merge branch 'master' into feat/5200/allow-moving-teams-of-integratio…
Dschoordsch Nov 11, 2021
d73485b
Clear freshly added integration auth from dataLoader
Dschoordsch Nov 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/client/modules/outcomeCard/components/OutcomeCardFooter/TaskFooter.tsx
View file
Open in desktop
Expand Up @@ -84,17 +84,22 @@ const TaskFooter = (props: Props) => {
const showTeam = area === USER_DASH
const {content, id: taskId, error, integration, tags, userId} = task
const isArchived = isTaskArchived(tags)
const canAssign = !integration && !isArchived
const canAssignUser = !integration && !isArchived
const canAssignTeam = !isArchived
return (
<React.Fragment>
<Footer>
<AvatarBlock>
{showTeam ? (
<TaskFooterTeamAssignee canAssign={canAssign} task={task} useTaskChild={useTaskChild} />
<TaskFooterTeamAssignee
canAssign={canAssignTeam}
task={task}
useTaskChild={useTaskChild}
/>
) : (
<TaskFooterUserAssignee
area={area}
canAssign={canAssign}
canAssign={canAssignUser}
cardIsActive={cardIsActive}
task={task}
useTaskChild={useTaskChild}
Expand Down
6 changes: 1 addition & 5 deletions packages/client/mutations/UpdateTaskMutation.ts
View file
Open in desktop
Expand Up @@ -104,7 +104,7 @@ const UpdateTaskMutation: StandardMutation<TUpdateTaskMutation, OptionalHandlers
updateTaskTaskUpdater(payload, {atmosphere, store: store as any})
},
optimisticUpdater: (store) => {
const {id, content, teamId, userId} = updatedTask
const {id, content, userId} = updatedTask
const task = store.get(id)
if (!task) return
const now = new Date()
Expand All @@ -113,10 +113,6 @@ const UpdateTaskMutation: StandardMutation<TUpdateTaskMutation, OptionalHandlers
updatedAt: now.toJSON()
}
updateProxyRecord(task, optimisticTask)
if (teamId) {
task.setValue(teamId, 'teamId')
task.setLinkedRecord(store.get(teamId)!, 'team')
}
if (userId) {
task.setValue(userId, 'userId')
task.setLinkedRecord(store.get(userId)!, 'user')
Expand Down
1 change: 0 additions & 1 deletion packages/client/types/graphql.ts
View file
Open in desktop
Expand Up @@ -58942,7 +58942,6 @@ export interface IUpdateTaskInput {
content?: string | null;
sortOrder?: number | null;
status?: TaskStatusEnum | null;
teamId?: string | null;

/**
* userId, the owner of the task. This can be null if the task is not assigned to anyone.
Expand Down
27 changes: 26 additions & 1 deletion packages/server/graphql/mutations/changeTaskTeam.ts
View file
Open in desktop
Expand Up @@ -50,6 +50,7 @@ export default {
}

// RESOLUTION
// filter mentions of old team members from task content
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const [oldTeamMembers, newTeamMembers] = await dataLoader
.get('teamMembersByTeamId')
.loadMany([oldTeamId, teamId])
Expand All @@ -69,7 +70,18 @@ export default {
updatedAt: now,
teamId
}
await r({

// If there is a task with the same integration hash in the new team, then delete it first.
// This is done so there are no duplicates and also solves the issue of the conflicting task being
// private or archived.
const {deletedConflictingIntegrationTask} = await r({
deletedConflictingIntegrationTask:
task.integrationHash &&
r
.table('Task')
.getAll(task.integrationHash, {index: 'integrationHash'})
.filter({teamId})
.delete({returnChanges: true}),
Dschoordsch marked this conversation as resolved.
Show resolved Hide resolved
newTask: r
.table('Task')
.get(taskId)
Expand All @@ -92,6 +104,19 @@ export default {
})
}).run()

const deletedTasks = (deletedConflictingIntegrationTask as any)?.changes?.map(
({old_val}) => old_val
)
deletedTasks?.forEach((task) => {
const isPrivate = task.tags.includes('private')
const data = {task}
newTeamMembers.forEach(({userId}) => {
if (!isPrivate || userId === task.userId) {
publish(SubscriptionChannel.TASK, userId, 'DeleteTaskPayload', data, subOptions)
}
})
})

const isPrivate = tags.includes('private')
const data = {taskId}
const teamMembers = oldTeamMembers.concat(newTeamMembers)
Expand Down
18 changes: 4 additions & 14 deletions packages/server/graphql/mutations/updateTask.ts
View file
Open in desktop
Expand Up @@ -25,7 +25,6 @@ type UpdateTaskInput = {
content?: string | null
sortOrder?: number | null
status?: TaskStatusEnum | null
teamId?: string | null
userId?: string | null
}
type UpdateTaskMutationVariables = {
Expand Down Expand Up @@ -57,14 +56,7 @@ export default {
const viewerId = getUserId(authToken)

// VALIDATION
const {
id: taskId,
teamId: inputTeamId,
userId: inputUserId,
status,
sortOrder,
content
} = updatedTask
const {id: taskId, userId: inputUserId, status, sortOrder, content} = updatedTask
const validContent = normalizeRawDraftJS(content)
const task = await r
.table('Task')
Expand All @@ -75,12 +67,11 @@ export default {
}
const {teamId, userId} = task
const nextUserId = inputUserId === undefined ? userId : inputUserId
const nextTeamId = inputTeamId || teamId
if (!isTeamMember(authToken, teamId) || !isTeamMember(authToken, nextTeamId)) {
if (!isTeamMember(authToken, teamId)) {
return standardError(new Error('Team not found'), {userId: viewerId})
}
if (inputTeamId || inputUserId) {
const error = await validateTaskUserIsTeamMember(nextUserId, nextTeamId, dataLoader)
if (teamId || inputUserId) {
const error = await validateTaskUserIsTeamMember(nextUserId, teamId, dataLoader)
if (error) {
return standardError(new Error('Invalid user ID'), {userId: viewerId})
}
Expand All @@ -90,7 +81,6 @@ export default {
updatedTask.sortOrder !== undefined && Object.keys(updatedTask).length === 2
const nextTask = new Task({
...task,
teamId: nextTeamId,
userId: nextUserId,
status: status || task.status,
sortOrder: sortOrder || task.sortOrder,
Expand Down
1 change: 0 additions & 1 deletion packages/server/graphql/types/UpdateTaskInput.ts
View file
Open in desktop
Expand Up @@ -17,7 +17,6 @@ const UpdateTaskInput = new GraphQLInputObjectType({
content: {type: GraphQLString},
sortOrder: {type: GraphQLFloat},
status: {type: TaskStatusEnum},
teamId: {type: GraphQLID},
userId: {
type: GraphQLID,
description:
Expand Down