[GH-669]: "Add org selector to create issue modal" thoratvinod:issue-669 #766

cyrusjc · 2024-04-09T06:19:08Z

Fixes #669
Takes #675 to the finish line.

mattermost-build · 2024-04-09T06:19:12Z

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

hanzei · 2024-04-09T06:55:14Z

@raghavaggarwal2308 @ayusht2810 Can you please review the community contribution?

codecov-commenter · 2024-04-09T06:57:55Z

Codecov Report

Attention: Patch coverage is 2.00000% with 98 lines in your changes are missing coverage. Please review.

Project coverage is 15.93%. Comparing base (da4c4df) to head (aecfd27).
Report is 3 commits behind head on master.

❗ Current head aecfd27 differs from pull request most recent head a95d58b

Please upload reports for the commit a95d58b to get more accurate results.

Files	Patch %	Lines
server/plugin/api.go	2.00%	98 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #766      +/-   ##
==========================================
- Coverage   16.16%   15.93%   -0.23%     
==========================================
  Files          17       17              
  Lines        6021     6117      +96     
==========================================
+ Hits          973      975       +2     
- Misses       5003     5097      +94     
  Partials       45       45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ayusht2810

Thanks for the contribution! @cyrusjc
I have just a few suggestions and clarifications.

server/plugin/api.go

webapp/src/components/github_repo_selector/github_repo_selector.jsx

ayusht2810 · 2024-04-17T15:58:14Z

webapp/src/components/github_repo_selector/github_repo_selector.jsx

+        }
+    }
+
+    onChangeForOrg = (_, org) => {


Can't we remove the first parameter here, if it is not used anywhere?

We could probably change it... the signature here is the same before changes:

mattermost-plugin-github/webapp/src/components/github_repo_selector/github_repo_selector.jsx

Lines 36 to 39 in 6cc3429

onChange = (_, name) => {

const repo = this.props.yourRepos.find((r) => r.full_name === name);

this.props.onChange({name, permissions: repo.permissions});

}

Not sure exactly why it was written this way but I'll remove placeholder from here and the other onChange signature mention in the another suggestion

Just tested removing the first parameter and it seems like it 's vital to the functionality.

Can you please tell us what errors or issues you had after making the changes?

The repository now displays what I think are the local files on the server after removing the first parameter as shown:

Suggested change

onChangeForOrg = (_, org) => {

onChangeForOrg = (org) => {

webapp/src/components/github_repo_selector/github_repo_selector.jsx

mattermost-build · 2024-04-30T01:02:40Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

ayusht2810 · 2024-04-30T11:13:16Z

server/plugin/api.go

-		c.Log.WithError(err).Warnf("Failed to list repositories")
-		p.writeAPIError(w, &APIErrorResponse{Message: "Failed to fetch repositories", StatusCode: http.StatusInternalServerError})
+		err := errors.New("Organization query param is empty")
+		c.Log.WithError(err).Warnf("Organization query param is empty")


Why have we made this change? We will be printing the same message twice in the logs with this change.

You're right, I've changed it to the following:

Suggested change

c.Log.WithError(err).Warnf("Organization query param is empty")

c.Log.Warnf("Organization query param is empty")

I'm not sure what best practices are here but I haven't seen an example of context logging without an error. Would this be okay?

ayusht2810 · 2024-04-30T11:15:02Z

server/plugin/api.go

-		p.writeAPIError(w, &APIErrorResponse{Message: "Failed to fetch repositories", StatusCode: http.StatusInternalServerError})
+		err := errors.New("Organization query param is empty")
+		c.Log.WithError(err).Warnf("Organization query param is empty")
+		p.writeAPIError(w, &APIErrorResponse{Message: "Bad request, must include organization name ", StatusCode: http.StatusBadRequest})


I don't think we need to write Bad request in the error, as it is already stated in the status code. Also, can we use the message as present in line 1289, as it tells us where we must include the organization name? What are your thoughts on this?

I think you make a good point, we can just use the logging message as it's more descriptive. I've changed it include the message in line 1289.

Suggested change

p.writeAPIError(w, &APIErrorResponse{Message: "Bad request, must include organization name ", StatusCode: http.StatusBadRequest})

p.writeAPIError(w, &APIErrorResponse{Message: "Organization query is empty, must include organization name ", StatusCode: http.StatusBadRequest})

server/plugin/api.go

webapp/src/action_types/index.js

mattermost-build · 2024-05-26T01:02:33Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

mickmister

Thanks for this contribution @cyrusjc 👍 LGTM

There are some merge conflicts caused by typescript migration. Can you please take a look at this? Thank you

cyrusjc · 2024-05-30T09:59:30Z

Ah I did a woopsies. I'll reopen this in a bit

cyrusjc requested review from hanzei and mickmister as code owners April 9, 2024 06:19

mattermost-build added the Contributor label Apr 9, 2024

hanzei requested review from ayusht2810 and raghavaggarwal2308 and removed request for mickmister and hanzei April 9, 2024 06:54

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Apr 9, 2024

ayusht2810 reviewed Apr 17, 2024

View reviewed changes

mattermost-build added the Lifecycle/1:stale label Apr 30, 2024

hanzei requested a review from ayusht2810 April 30, 2024 09:28

hanzei removed the Lifecycle/1:stale label Apr 30, 2024

ayusht2810 reviewed Apr 30, 2024

View reviewed changes

mattermost-build added the Lifecycle/1:stale label May 26, 2024

mickmister requested a review from ayusht2810 May 28, 2024 19:57

mickmister reviewed May 28, 2024

View reviewed changes

cyrusjc closed this May 30, 2024

cyrusjc force-pushed the cyrusjc/optimize-github-modal branch from 1845085 to e958064 Compare May 30, 2024 09:42

rebased changes + typescript reducers

a95d58b

cyrusjc reopened this May 31, 2024

mickmister self-requested a review May 31, 2024 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-669]: "Add org selector to create issue modal" thoratvinod:issue-669 #766

[GH-669]: "Add org selector to create issue modal" thoratvinod:issue-669 #766

cyrusjc commented Apr 9, 2024 •

edited

Loading

mattermost-build commented Apr 9, 2024

hanzei commented Apr 9, 2024

codecov-commenter commented Apr 9, 2024 •

edited

Loading

ayusht2810 left a comment

ayusht2810 Apr 17, 2024

cyrusjc Apr 18, 2024

cyrusjc Apr 18, 2024

ayusht2810 Apr 30, 2024

cyrusjc May 1, 2024

mattermost-build commented Apr 30, 2024

ayusht2810 Apr 30, 2024

cyrusjc May 1, 2024

ayusht2810 Apr 30, 2024

cyrusjc May 1, 2024

mattermost-build commented May 26, 2024

mickmister left a comment •

edited

Loading

cyrusjc commented May 30, 2024

	onChange = (_, name) => {
	const repo = this.props.yourRepos.find((r) => r.full_name === name);
	this.props.onChange({name, permissions: repo.permissions});
	}

	c.Log.WithError(err).Warnf("Organization query param is empty")
	c.Log.Warnf("Organization query param is empty")

	p.writeAPIError(w, &APIErrorResponse{Message: "Bad request, must include organization name ", StatusCode: http.StatusBadRequest})
	p.writeAPIError(w, &APIErrorResponse{Message: "Organization query is empty, must include organization name ", StatusCode: http.StatusBadRequest})

[GH-669]: "Add org selector to create issue modal" thoratvinod:issue-669 #766

Are you sure you want to change the base?

[GH-669]: "Add org selector to create issue modal" thoratvinod:issue-669 #766

Conversation

cyrusjc commented Apr 9, 2024 • edited Loading

mattermost-build commented Apr 9, 2024

hanzei commented Apr 9, 2024

codecov-commenter commented Apr 9, 2024 • edited Loading

Codecov Report

ayusht2810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermost-build commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermost-build commented May 26, 2024

mickmister left a comment • edited Loading

Choose a reason for hiding this comment

cyrusjc commented May 30, 2024

cyrusjc commented Apr 9, 2024 •

edited

Loading

codecov-commenter commented Apr 9, 2024 •

edited

Loading

mickmister left a comment •

edited

Loading