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.

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

feat(ado): Sprint Poker Estimates #6481

Merged
merged 195 commits into from
May 11, 2022
Merged

Conversation

bobjac
Copy link
Contributor

@bobjac bobjac commented Apr 28, 2022

This pull request includes functionality for importing Azure DevOps work items into a sprint poker meeting and pushing estimates back to Azure Dev Ops. It includes:

  • Import user stories from Azure DevOps. The backend code will look at the OAuth2 token provided by Azure Active Directory and query across all tenants and projects to find user stories the user is able to access.
  • Those user stories can be imported
  • More granular work items (tasks, bugs) can be found through search which is implemented in another PR by @danielkon96
  • Once the stories are imported, the can be estimated in the Poker meeting. You can select the field you wish to push the estimates to (comment or base field for the work item type)

Screen Shot 2022-04-28 at 1 47 53 PM
![Screen Shot 2022-04-28 at 1 48 37 PM](https://user-images.githubusercontent.com/1582053/165815719-effb2b85-19e3-4163-a85c-c05c409
Screen Shot 2022-04-28 at 1 48 58 PM
a552b.png)
Screen Shot 2022-04-28 at 1 49 28 PM
Screen Shot 2022-04-28 at 1 49 45 PM
Screen Shot 2022-04-28 at 1 50 12 PM
Screen Shot 2022-04-28 at 1 50 41 PM
Screen Shot 2022-04-28 at 1 51 05 PM

bobjac and others added 30 commits March 14, 2022 01:48
…sIssue to schema and corresponding artifacts
…abolInc/parabol into feature/6125/ado-integration

Pulling commits from the feature/6125/ado-integration branch
@bobjac bobjac requested a review from nickoferrall May 5, 2022 20:04
@bobjac
Copy link
Contributor Author

bobjac commented May 6, 2022

Looks like some merge conflicts were introduced after the query PR was accepted/merged. I am resolving now.

@bobjac
Copy link
Contributor Author

bobjac commented May 6, 2022

Merge conflicts should be resolved

Copy link
Contributor

@nickoferrall nickoferrall left a comment

Choose a reason for hiding this comment

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

A few issues I've noticed while playing around with the integrations. Feel free to create new issues for the first two:

  1. When I returned to the meeting, I couldn't see any issues. My server logs told me that I needed to refresh the token. How can we fix this so that users won't need to refresh their tokens so frequently?

  2. If I click on the hyperlink in the Estimate task card (First pass of user onboarding journey #2 in screenshot below), it doesn't take me to the Azure Devops project:
    Screenshot 2022-05-06 at 16 14 58

  3. After pulling the latest changes after you fixed the merge conflicts, I get a server error when pushing a score to any Azure Estimate item.

16:19:26 1|GraphQL Executor  | Trace: {
16:19:26 1|GraphQL Executor  |   error: '{"data":null,"errors":[{"message":"column \\"azureDevOpsFieldName\\" of relation \\"TaskEstimate\\" does not exist","locations":[{"line":4,"column":3}],"path":["setTaskEstimate"]}]}'
16:19:26 1|GraphQL Executor  | }
16:19:26 1|GraphQL Executor  |     at executeGraphQL (/Users/nickoferrall/Parabol/dev/gqlExecutor.js:524:13)
16:19:26 1|GraphQL Executor  |     at processTicksAndRejections (node:internal/process/task_queues:96:5)
16:19:26 1|GraphQL Executor  |     at async onMessage (/Users/nickoferrall/Parabol/dev/gqlExecutor.js:98113:22)
16:19:26 2|Socket Server     | SEND TO SENTRY [Error: column "azureDevOpsFieldName" of relation "TaskEstimate" does not exist] undefined
  1. This is unrelated to this PR, but in the Scope Search we have a placeholder Search issues on Azure DevOps. I just wanted to check whether everything would be referred to as an issue in Azure? I know we return issues but also tasks, epics, and user stories.

@bobjac
Copy link
Contributor Author

bobjac commented May 6, 2022

Hey @nickoferrall - For item #3 (the azureDevOpsFieldName of relation 'TaskEstimate'), this is happening because @wdfox fixed a spelling type to the database column. @wdfox can confirm here, but I believe you will need to re-setup your local database container to run the migration.

I planned on creating separate issues for the first 2. For item #2, there is a hypermedia field in the Azure DevOps api that returns the html url. I was planning on expanding the graph schema and capturing the html url on the work item so it could be displayed as that link. The standard url, that is showing, is the hypermedia url mean for consumption by REST clients.

For the authentication/refresh token.....AAD / AzureDevOps has an expiration on the refresh token where JIRA doesn't. At some point (which can be controlled by policy up to a certain amount), the refresh token is going to expire and the user will be forced to re-authenticate.

@bobjac
Copy link
Contributor Author

bobjac commented May 6, 2022

@nickoferrall - for you comment on #4 (Search issues on Azure DevOps). An issue in Azure DevOps is a work item type. In GitHub, the base type is Issue and labels are applied (bug, enancement). In Azure DevOps, the was type is work item and there are actual inherited types from that (User Story, Issue, bug) and those inherited types are different depending on the project type. So, there are "Issues" but it would make more sense from an Azure DevOps perspective to say "Work Items".

I kept issues there because I was thinking in terms of how Parabol sees the base type. I wasn't sure if parabol users consider everything an Issue and when you import from an external system, that issue is really a pointer to the type in the external system.

It is an easy change that I would be happy to make if you think it should say "Work Items" in the case of Azure Dev Ops.

Copy link
Contributor

@nickoferrall nickoferrall left a comment

Choose a reason for hiding this comment

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

LGTM! For the problem where we can't select the "effort" label for Epics, is this the issue that will resolve that: #6512 ?

And I just wanted to check if you've created issues for the first and second points mentioned above?

Thanks for the detail on the fourth point. We want our integrations to feel similar to the app that they're used to using. If "Work Items" is what they're called in Azure, it'd be great to refer to them as "Work Items" in Parabol's app too.

@nickoferrall
Copy link
Contributor

Ready for a review from @Dschoordsch now

@wdfox
Copy link
Contributor

wdfox commented May 6, 2022

Bob is spot on about issue #3, though you can also resolve this directly by pulling up PGAdmin and directly fixing the spelling of the column named 'azureDevOpsFieldName' in the TaskEstimate table

@bobjac bobjac linked an issue May 6, 2022 that may be closed by this pull request
@bobjac
Copy link
Contributor Author

bobjac commented May 6, 2022

Ready for a review from @Dschoordsch now

@nickoferrall - do you need anything from me to advance to the next stage, or will @Dschoordsch be notified automatically?

@nickoferrall
Copy link
Contributor

@nickoferrall - do you need anything from me to advance to the next stage, or will @Dschoordsch be notified automatically?

Nope, nothing you need to do. Georg has already been notified 👍

Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Some smaller issues. Everything except the migration could also be fixed in a later PR.

While testing I saw at least 2 issues:

  • the issue links are wrong and lead to an API instead of the normal project page
    Issue link leads to API
  • the field "Original Estimate" is also shown for Basic tasks which don't have that field
    Original Estimate field for Basic task

packages/client/components/AzureDevOpsFieldMenu.tsx Outdated Show resolved Hide resolved
packages/client/components/AzureDevOpsFieldMenu.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
const AzureDevOpsDimensionFieldId = {
join: (instanceId: string, dimensionName: string, fieldId: string) =>
`${instanceId}:${dimensionName.replace(/\s/g, '¶')}:${fieldId}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I would be more interested why we need to replace whitespace

packages/client/types/constEnums.ts Outdated Show resolved Hide resolved
packages/server/graphql/types/AzureDevOpsDimensionField.ts Outdated Show resolved Hide resolved
@@ -233,6 +256,24 @@ class AzureDevOpsServerManager {
return json
}

private readonly patch = async <T>(url: string, payload: any) => {
const patchHeaders = this.headers
patchHeaders['Content-Type'] = 'application/json-patch+json'
Copy link
Contributor

Choose a reason for hiding this comment

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

-1 Since you're only assigning a reference in line 260 this will modify this.headers which is most likely not what you want. Instead it would be easier if you would modify the next lines

    const res = await this.fetchWithTimeout(url, {
      ...
      headers: {
        ...this.headers,
        ['Content-Type']: 'application/json-patch+json'
      }
      ...
    })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wdfox - I agree with @Dschoordsch. I just wanted to confirm, you weren't looking to permanently modify the Content Type of any future calls to Azure DevOps. You only wanted the json-path when calling patch, correct?

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 made the change. I just want confirmation from @wdfox that this was the intent. I will mark resolved when confirmed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that is correct @bobjac. This patch endpoints requires the Content-Type to be application/json-patch+json. I made a quick fix the easiest way I could think, but your modification should be perfectly fine.

@bobjac
Copy link
Contributor Author

bobjac commented May 10, 2022

Great. I'll take a look this afternoon.

@bobjac
Copy link
Contributor Author

bobjac commented May 11, 2022

@Dschoordsch - the two issues you pointed out in the top comment have separate issues for them already.

  1. For the link to the JSON object, there is an html link returned from the hypermedia REST api. I am either going to add it as a new graphql property or replace the URL with the html link.
  2. The basic task property is going to be fixed by code I have stashed. When testing, we need to account for the project when considering the work item type. For example , in a Scrum project, the "bug" work item type will have different available fields than the "bug" work item type in the Agile project. I wrote the code to capture the available fields based off the project/type combination.

@bobjac
Copy link
Contributor Author

bobjac commented May 11, 2022

There was no issue to resolve in the above comment referenced at #6481 (comment), but this was copied from the previous atlassian implementation.

Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

There is a small issue: when first switching to a field and voting directly (as comments is pre-selected) it fails with an error message
First estimate to comment fails
Safe to merge anyways since it's behind a feature flag.

@Dschoordsch Dschoordsch merged commit 73ec630 into master May 11, 2022
@Dschoordsch Dschoordsch deleted the feature/6434/ado-estimates branch May 11, 2022 12:48
@bobjac
Copy link
Contributor Author

bobjac commented May 11, 2022

@Dschoordsch - that last issue was also related to the project/work item relationship. I have an issue open for that. I will create a branch and fix that since this has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(Integrations): Push Poker Meeting estimates to Azure DevOps
6 participants