[GH-669]: "Add org selector to create issue modal" thoratvinod:issue-669 by cyrusjc · Pull Request #766 · mattermost/mattermost-plugin-github

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.

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

+        }
+    }
+
+    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) => {

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

-		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

-		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})

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

wiggin77 · 2024-10-22T22:45:55Z

@Kshitij-Katiyar how does this PR relate to your recent PR handling orgs?

raghavaggarwal2308 · 2024-10-23T16:24:37Z

@wiggin77 The PR created by us is just fixing an issue in the existing multiple org functionality on master (not released yet) and this PR is adding the functionality to just the create issue modal:

In the multiple org feature, we added a functionality to add multiple orgs in the plugin settings and we get the data of the orgs selected in the system console in the create issue modal dropdown (and all the other places in the plugin). But in case no org is selected, we will have the data from all the orgs in the user's account.
The feature implemented in this PR adds a dropdown that has all the orgs listed in a dropdown and the user can filter the repos dropdown on the basis of that. Also, this PR makes it so that if an org (singular org i.e. behaviour before multi org PR was merged) is set in the plugin config then only that org will be listed in the dropdown.

To make this PR work with the multiple org feature we will have to make some tweaks since earlier there was only one org allowed in the config but now that behavior is changed to list multiple orgs.

wiggin77 · 2025-01-15T19:37:18Z

To make this PR work with the multiple org feature we will have to make some tweaks since earlier there was only one org allowed in the config but now that behavior is changed to list multiple orgs.

@raghavaggarwal2308 How much/time effort is needed to get this working with the current master branch?

raghavaggarwal2308 · 2025-01-17T09:52:32Z

@wiggin77 I think it will take around 1 day to get this working with the master branch. Also, we will have to close this PR and create a new one, since this is created from a separate fork.

wiggin77 · 2025-03-24T21:03:31Z

@raghavaggarwal2308 let me know if/when this becomes ready for review.

…ub into cyrusjc/optimize-github-modal

Kshitij-Katiyar · 2025-05-01T10:42:06Z

@wiggin77 I have resolved the conflicts on this PR, You can have a look

wiggin77 · 2025-05-01T12:39:48Z

+		githubClient := p.githubConnectUser(c.Context.Ctx, c.GHInfo)
+		orgList, err := getOrganizationList(c.Ctx, "", githubClient, github.ListOptions{PerPage: 50})
+		if err != nil {
+			c.Log.WithError(err).Warnf("Failed to list organizations")


Shouldn't this be an error level log? It is returning a 500 error on the request.

Same with all the other places a 500 error is written on the response.

@cyrusjc is not actively working on this PR so I will be fixing these issuse

@wiggin77 I have fixed these issues at multiple places

wiggin77 · 2025-05-01T12:42:26Z

@wiggin77 I have resolved the conflicts on this PR, You can have a look

@Kshitij-Katiyar there is a problem with the logging levels.

Kshitij-Katiyar · 2025-05-06T10:56:56Z

@wiggin77 I have resolved the conflicts on this PR, You can have a look

@Kshitij-Katiyar there is a problem with the logging levels.

@wiggin77 Done with the fixes

AayushChaudhary0001

The PR is tested, and code changes are working fine on adding an organisation selector in the create issue modal to filter the repositories based on organisations for both MM local and cloud server, including public and private repositories.

LGTM, Approved.

Merge upstream mattermost-plugin-github (…4b03e0c, ~40 commits up to just before the React 18 migration) into the Forgejo fork. Resolved conflicts case-by-case: upstream logic by default, Forgejo specifics preserved. Key resolutions / deviations from upstream: - Telemetry removed (upstream mattermost#952): experimental/telemetry was dropped from server/public v0.1.21, so telemetry.go deleted and all tracker call sites removed (api.go, flows.go, plugin.go). Forced by the dependency, not a choice. - graphql/ stays removed (fork already dropped it); go.mod tidied (shurcooL deps). - Ported useGitHubClient + handleRevokedToken into plugin.go (Forgejo wording) so upstream's token-revocation handling works; wired into label/assignee/ milestone fetchers. - Brought in org selector endpoints mattermost#766 (getOrganizations/getReposByOrg/ getRepository, getOrganizationList, RepoResponse/RepositoryResponse types), adapted to Forgejo names and the fork's package-level getRepositoryList/ getRepositoryListByOrg signatures. - Brought in default-repo command mattermost#938 (handlers + DefaultRepoKey/ErrNotFound), wired "default-repo" into CommandHandlers and autocomplete (was unused). - Adopted upstream log-spam fix mattermost#951 and connect error propagation mattermost#934 with Forgejo wording; kept the fork's refresh-token decryption path. - link_tooltip: openedByLink prefers user.html_url, falls back to the configured Forgejo instance (forgejo.pyn.ru) instead of github.com. - webapp TS migrations: ported Forgejo renames into the new client.ts and user_attribute.ts/.tsx (getForgejoUser, forgejoUsers, baseURL, fa-git). - post-menu create-issue/attach-comment adopted upstream's functional component form (registerPostDropdownMenuAction model) with Forgejo naming. Tests: kept the fork's api_test/command_test/webhook_test (upstream's new github-shaped suites mattermost#855 not ported in M1); updated help-command test (help no longer fetches user info) and link_tooltip fallback test for Forgejo. Gate: go build/vet/test, golangci-lint v2, webapp build/tsc/eslint/jest, make dist — all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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 hanzei and mickmister 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

raghavaggarwal2308 requested a review from wiggin77 July 5, 2024 08:28

mickmister removed their request for review July 10, 2024 05:23

wiggin77 removed request for ayusht2810 and wiggin77 March 24, 2025 21:03

Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…

14be861

…ub into cyrusjc/optimize-github-modal

Kshitij-Katiyar requested a review from wiggin77 as a code owner May 1, 2025 10:21

resolved manifest import in gihub repo selector

c59c27f

wiggin77 requested changes May 1, 2025

View reviewed changes

Kshitij-Katiyar added 2 commits May 6, 2025 15:46

fix logs

4aeac59

fixed testcases

c2c8d72

wiggin77 approved these changes May 6, 2025

View reviewed changes

Kshitij-Katiyar requested a review from avas27JTG May 7, 2025 07:32

wiggin77 requested review from Kshitij-Katiyar and ayusht2810 May 10, 2025 17:25

avas27JTG reviewed May 19, 2025

View reviewed changes

Comment thread webapp/src/types/github_types.ts

review fixes

a56308c

Kshitij-Katiyar approved these changes May 21, 2025

View reviewed changes

AayushChaudhary0001 approved these changes May 23, 2025

View reviewed changes

Kshitij-Katiyar merged commit c1ac2f9 into mattermost:master May 23, 2025

hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale 3: QA Review Requires review by a QA tester labels May 23, 2025

	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})

Uh oh!

Conversation

cyrusjc commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattermost-build commented Apr 9, 2024

Uh oh!

hanzei commented Apr 9, 2024

Uh oh!

codecov-commenter commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ayusht2810 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattermost-build commented Apr 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattermost-build commented May 26, 2024

Uh oh!

mickmister left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyrusjc commented May 30, 2024

Uh oh!

wiggin77 commented Oct 22, 2024

Uh oh!

raghavaggarwal2308 commented Oct 23, 2024

Uh oh!

wiggin77 commented Jan 15, 2025

Uh oh!

raghavaggarwal2308 commented Jan 17, 2025

Uh oh!

wiggin77 commented Mar 24, 2025

Uh oh!

Kshitij-Katiyar commented May 1, 2025

Uh oh!

wiggin77 May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiggin77 commented May 1, 2025

Uh oh!

Kshitij-Katiyar commented May 6, 2025

Uh oh!

Uh oh!

cyrusjc commented Apr 9, 2024 •

edited

Loading

codecov-commenter commented Apr 9, 2024 •

edited

Loading

mickmister left a comment •

edited

Loading

wiggin77 May 1, 2025 •

edited

Loading