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

Jira integration for release readiness #682

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Jira integration for release readiness #682

wants to merge 4 commits into from

Conversation

gitstart-app[bot]
Copy link
Contributor

@gitstart-app gitstart-app bot commented Nov 20, 2024

Implementation

We implemented a Jira API layer integrated with Atlassian’s OAuth2, allowing secure access to Jira data. Users can select their Jira organization during setup and configure multiple projects with custom “done” states and release filters (labels or fix versions).

The JiraIntegration model manages API calls for fetching projects, workflows, and tickets. We added “Project Management” as a new integration category in Tramline, with Jira as the first platform. The functionality, including OAuth flow, project setup, and ticket fetching, is thoroughly tested with fixtures and unit tests.

### Demo

https://www.loom.com/share/14b9a786dfa742d194f5f079d52b4766?sid=9554c7d6-414c-4f51-9caa-74004b473af1

@gitstart-tramline
Copy link

Hi @kitallis, PR is ready for review. Please review.

@gitstart-tramline
Copy link

Here are the steps to set up the Jira integration

To create an app for Jira integration and obtain the client_id and `client_secret, follow these steps:
Step 1: Create a Developer Account

Step 2: Create a New App

  • Navigate to the Developer Console.
  • Click on Create App and select the option for OAuth 2.0.
  • Provide the following details:
    • App Name
    • App Description
    • API Permissions:
      • Required permissions: read:jira-work write:jira-work read:jira-user offline_access

Step 3: Set Redirect URLs

Locate the client_id and client_secret in Settings.

Copy link
Member

@kitallis kitallis left a comment

Choose a reason for hiding this comment

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

I think functionally this works well. I will hack on the UI/UX bits myself in this PR to make it more consistent with Tramline, other than that, I've left various comments inline.

Thank you.

Comment on lines +20 to +30
else
@resources = @integration.providable.available_resources

if @resources.blank?
redirect_to app_integrations_path(state_app),
alert: t("integrations.project_management.jira.no_organization")
return
end

render "jira_integration/select_organization"
end
Copy link
Member

Choose a reason for hiding this comment

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

question: what scenario is this? afaik we select organization in the jira redirect itself, so why would we need to select again inside Tramline?

Choose a reason for hiding this comment

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

The Atlassian API endpoint https://api.atlassian.com/oauth/token/accessible-resources returns a list of all resources (organizations) the user has access to after authentication.
This is because:

  • Multiple Resources: The API does not automatically limit the response to the organization selected during the redirect. Instead, it provides all accessible organizations for the user.

  • User Selection in Tramline: To ensure the correct organization is used in Tramline, the app must present the user with the list of accessible organizations returned by the API. The user then selects the desired organization explicitly within the app.

@@ -0,0 +1,37 @@
import { Controller } from "@hotwired/stimulus"
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: generally, we should try and create generic stimulus controllers as much as possible, because they are reusable.

even if that's not possible, we should avoid putting hyper-specific element names like jira_config inside the controller, because it not only makes it not reusable, but it also makes it non-reusable for non-jira project selections.

Choose a reason for hiding this comment

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

we made the controller generic and removed the specific names

Comment on lines 1 to 17
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
static targets = ["template", "container", "filter"]

add(event) {
event.preventDefault()
const content = this.templateTarget.innerHTML.replace(/__INDEX__/g, this.filterTargets.length)
this.containerTarget.insertAdjacentHTML("beforeend", content)
}

remove(event) {
event.preventDefault()
const filter = event.target.closest("[data-release-filters-target='filter']")
filter.remove()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I think this functionality is partly already covered in the nested_form_ext_controller.js – it supports generically creating list items and then also integrates positional dragging optionally.

see if we can reuse that controller here.

Choose a reason for hiding this comment

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

The logic differs between the two controllers. The super.add and super.remove methods in the NestedForm controller call the parent NestedForm class's add and remove methods, which handle the default behavior for adding and removing nested form elements. In contrast, the release_filters_controller.js manages basic DOM manipulations (inserting and removing content) without handling positional management or nested form logic. Therefore, reusing the NestedForm controller in this case may not be appropriate.

Comment on lines 1 to 10
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
static targets = ["content"]

toggle(event) {
this.contentTarget.style.display = event.target.checked ? "block" : "none"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: same as before, this is too trivial and general as an idea, we should make this functionality generic, additionally, we might have a controller that does something like this – just take a look at our current stimulus controllers.

host = request.host_with_port
Rails.application.routes.url_helpers.jira_callback_url(
host: host,
protocol: request.protocol.gsub("://", "")
Copy link
Member

Choose a reason for hiding this comment

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

question: what is this gsub for?

Choose a reason for hiding this comment

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

the gsub is used to remove the //: always present on the protocol

Copy link
Member

Choose a reason for hiding this comment

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

suggestion: we should avoid using jira-specific terminology in this view, we should fork off jira-specific code into its own partials (see other views/app_configs/* for reference)

Choose a reason for hiding this comment

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

got it

Comment on lines 151 to 159
with_api_retries do
response = api.search_tickets_by_filters(
project_key,
release_filters
)

return [] if response["issues"].blank?

Installations::Response::Keys.transform(response["issues"], TICKET_TRANSFORMATIONS)
Copy link
Member

Choose a reason for hiding this comment

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

thought: I think we should avoid using the transformations directly in the integration layer if possible. The general pattern that we use is to define the transformations in this layer and then pass them over to the api layer. This ensures that the output from the api layer is consistent across integrations

Choose a reason for hiding this comment

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

makes sense

Comment on lines 216 to 219
with_api_retries do
response = api.projects
projects = Installations::Response::Keys.transform(response, PROJECT_TRANSFORMATIONS)
{projects: projects}
Copy link
Member

Choose a reason for hiding this comment

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

thought: see previous comment about transformations

def extract_unique_statuses(statuses)
statuses.flat_map { |issue_type| issue_type["statuses"] }
.uniq { |status| status["id"] }
.then { |statuses| Installations::Response::Keys.transform(statuses, STATUS_TRANSFORMATIONS) }
Copy link
Member

Choose a reason for hiding this comment

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

thought: see previous comment about transformations

Comment on lines 123 to 144
def build_jql_query(project_key, release_filters)
conditions = ["project = '#{sanitize_jql_value(project_key)}'"]

release_filters.each do |filter|
value = sanitize_jql_value(filter["value"])

case filter["type"]
when "label"
conditions << "labels = '#{value}'"
when "fix_version"
conditions << "fixVersion = '#{value}'"
else
Rails.logger.warn("Unsupported Jira filter type: #{filter["type"]}")
end
end

conditions.join(" AND ")
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def build_jql_query(project_key, release_filters)
conditions = ["project = '#{sanitize_jql_value(project_key)}'"]
release_filters.each do |filter|
value = sanitize_jql_value(filter["value"])
case filter["type"]
when "label"
conditions << "labels = '#{value}'"
when "fix_version"
conditions << "fixVersion = '#{value}'"
else
Rails.logger.warn("Unsupported Jira filter type: #{filter["type"]}")
end
end
conditions.join(" AND ")
end
def build_jql_query(project_key, release_filters)
conditions = ["project = '#{sanitize_jql_value(project_key)}'"]
release_filters.each do |filter|
value = sanitize_jql_value(filter["value"])
filter_condition =
case filter["type"]
when "label" then "labels = '#{value}'"
when "fix_version" then "fixVersion = '#{value}'"
else Rails.logger.warn("Unsupported Jira filter type: #{filter["type"]}")
end
conditions << filter_condition if filter_condition
end
conditions.join(" AND ")
end

@gitstart-tramline
Copy link

Thank you for the review. We will work on the changes and get back to you.

@gitstart-app gitstart-app bot marked this pull request as draft January 8, 2025 09:33
@gitstart-app gitstart-app bot marked this pull request as ready for review January 8, 2025 12:13
@gitstart-tramline
Copy link

Hi @kitallis, PR is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants