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

#353 | Pull request reminder widget (github / bitbucket) #403

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mGasiorek998
Copy link
Collaborator

@mGasiorek998 mGasiorek998 commented Jun 25, 2021

#353 Pull request reminder widget (github / bitbucket)

Description

Added new widget which will show pull requests of a given repository.

Motivation and Context

User will be able to track all of the open pull request from a repository

Screenshots

Capture
Capture1

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • Automated functional tests have been added or modified to cover my changes (if applicable)
  • I have updated the documentation accordingly.

I hereby agree to the terms of the Cogboard Contributor License Agreement.

@mGasiorek998 mGasiorek998 self-assigned this Jun 25, 2021
@mGasiorek998 mGasiorek998 linked an issue Jun 25, 2021 that may be closed by this pull request
4 tasks
</PullRequestLink>
))
) : (
<StyledNoItemsInfo>
Copy link
Collaborator

@YellowFlash525 YellowFlash525 Jun 26, 2021

Choose a reason for hiding this comment

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

I think this part will never be rendered because of condition in line 14

Copy link
Collaborator Author

@mGasiorek998 mGasiorek998 Jun 28, 2021

Choose a reason for hiding this comment

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

Github API returns empty array when there is no pull requests in a given repository, so the widget will display "No Pull Requests", but if path to repository is invalid or API limit is reached it will return nothing, so widget will display "API rate limit exceeded!"

import styled from '@emotion/styled/macro';
import { COLORS } from '../../../../constants/';

export const PullRequestContainer = styled('div')`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const PullRequestContainer = styled('div')`
export const PullRequestContainer = styled.div`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in commit 53f7cc8

}
`;

export const PullRequestLink = styled('a')`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const PullRequestLink = styled('a')`
export const PullRequestLink = styled.a`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in commit 53f7cc8

YellowFlash525
YellowFlash525 previously approved these changes Jun 28, 2021
val apiUrl = url.replace("github.com/", "api.github.com")
httpGet(url = "$apiUrl/repos$path/pulls")
} else {
httpGet(url = "https://api.bitbucket.org/2.0/repositories$path/pullrequests")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to be able to setup some repos from here:
https://bitbucket.cognifide.com/dashboard

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in f03e91e

override fun updateState() {
if (url.isNotBlank() && path.isNotBlank()) {
if (url.contains("github")) {
val apiUrl = url.replace("github.com/", "api.github.com")
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work for commertial repos i think you should be replacing :// with ://api.

Copy link
Contributor

Choose a reason for hiding this comment

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

my project repo as an example: https://github.company.com/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in commit f03e91e

Comment on lines 27 to 31
<PullRequestLink key={id} href={url} target="_blank">
<PullRequestContainer>
{`${index + 1}. ${formatPullRequestTitle(title)}`}
</PullRequestContainer>
</PullRequestLink>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reuse the existing look & feel from other widgets - look at Random picker, Link List, ToDo list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in commit fe5980b

Copy link
Contributor

@szymon-owczarzak szymon-owczarzak left a comment

Choose a reason for hiding this comment

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

Please have a look @ my comments.
Add mocks, and example content for two widgets.
Test solution with our companys bitbucket.

@szymon-owczarzak szymon-owczarzak added the enhancement New feature or request label Jun 30, 2021
Comment on lines +24 to +29
{
"id": "endpoint5",
"label": "Bitbucket Cognifide",
"publicUrl": "https://bitbucket.cognifide.com/",
"url": "https://bitbucket.cognifide.com/",
"credentials": ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this and instead add support to our Api Mocks - and mimic a response there. Please make sure you are not providing any sensitiva data.

},
PullRequestReminderWidget: {
component: PullRequestReminderWidget,
dialogFields: ['EndpointField', 'SchedulePeriod', 'Path'],
Copy link
Contributor

@szymon-owczarzak szymon-owczarzak Jul 13, 2021

Choose a reason for hiding this comment

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

Let's expand dialog with new fields:
1 Project (Group) 2 Repository in place of Path
3 API Type dropdown with values: Bitbucket 1.0, Bitbucket 2.0, GitHub- we will expand if needed - GitHub as a default one

Copy link
Contributor

Choose a reason for hiding this comment

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

Use SonarQube Version field as an example

@@ -73,6 +73,8 @@ class CogboardConstants {
const val JQL_QUERY = "jqlQuery"
const val BUCKET_QUERIES = "bucketQueries"
const val BUCKET_NAME = "bucketName"
const val PULL_REQUESTS = "pullRequests"
const val REPOSITORY_HUB = "repositoryHub"
Copy link
Contributor

@szymon-owczarzak szymon-owczarzak Jul 13, 2021

Choose a reason for hiding this comment

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

after the introduction of apiType dropdown this will not be required.

override fun handleResponse(responseBody: JsonObject) {
var pullRequests = responseBody

if (url.contains("github")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use switch-case on apiType instead

pullRequests =
pullRequests
.put(Props.PULL_REQUESTS, responseBody.remove("array"))
.put(Props.REPOSITORY_HUB, "github")
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to send this property

Comment on lines +38 to +48
if (url.contains("github")) {
var (_, owner, repo) = path.split("/")
var apiUrl = url.replace("://", "://api.").replace("com/", "com")
httpGet("$apiUrl/repos/$owner/$repo/pulls")
} else if (url.contains("bitbucket") && path.contains("dashboard")) {
httpGet("$url/rest/api/1.0/dashboard/pull-requests")
return
} else {
var (_, project, repo) = path.split("/")
httpGet(url = "$url/rest/api/1.0/projects/$project/repos/$repo/pull-requests")
}
Copy link
Contributor

@szymon-owczarzak szymon-owczarzak Jul 13, 2021

Choose a reason for hiding this comment

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

move this logic to a seprate class so we can just:

httpGet(UrlUtil.getApiUrl(apiType, url, project, repository))

You can do this as an extension method on string class also.
httpGet(url.getApiUrl(apiType, project, repository))
https://kotlinlang.org/docs/extensions.html#extension-functions
write Unit tests for supported examples

Copy link
Contributor

@szymon-owczarzak szymon-owczarzak left a comment

Choose a reason for hiding this comment

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

Look at my comments and the screenshot below.
Let's make this widget more generic and future prooof

image

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

Successfully merging this pull request may close these issues.

Pull request reminder widget (github / bitbucket)
5 participants