[MM-420] Add feature to support multiple orgs in plugin settings by ayusht2810 · Pull Request #773 · mattermost/mattermost-plugin-github

ayusht2810 · 2024-04-30T05:55:06Z

Summary

Added feature to support multiple orgs in plugin settings
Updated Readme

Ticket Link

Fixes #552

What to test?

Add comma separated organizations name in plugin settings.
Test all the features required with the above change and check if we get all the details related to different organization

Checklist

Completed dev testing
make test Ran test cases and ensured they are passing
make check-style Ran style check and ensured both webapp and server pass the checks

hanzei

Looking forward to using this feature on Community!

hanzei · 2024-04-30T07:00:06Z

 	}
 }
+
+func TestGetOrganizations(t *testing.T) {


Table tests ❤️

hanzei · 2024-04-30T09:11:38Z

-	if len(org) != 0 {
-		orgField = fmt.Sprintf("org:%v", org)
+	for _, org := range orgs {
+		if len(org) != 0 {


Can org be ""?

Yes, if a user enters orgA,orgB, as values in the system console, the last value after splitting the string will be an empty string.

Should we guard for that case in getOrganizations?

Should we guard for that case in getOrganizations?

@hanzei Can you please elaborate more on this comment? We have already added the guard to the getOrganizations function. Do you mean to remove it from here and apply the condition separately, wherever it is required?

The slice returned from getOrganizations can never contain an empty string, right? Do we need the additional check here?

@hanzei There was an additional check present in the lhs_request.go file, which I have removed now. Now, we only have this check present in the getOrganizations function.

…pdate function definition and minor reveiw fixes

ayusht2810 · 2024-05-01T16:57:27Z

@hanzei fixed the review comments. Please re-review.

hanzei · 2024-05-02T10:51:17Z

-	if len(org) != 0 {
-		orgField = fmt.Sprintf("org:%v", org)
+	for _, org := range orgs {
+		if len(org) != 0 {


Should we guard for that case in getOrganizations?

…tion file

ayusht2810 · 2024-05-07T12:00:35Z

@hanzei Fixed the review comments. Please re-review

hanzei

LGTM 🚀

hanzei · 2024-05-07T19:48:24Z

-	if len(org) != 0 {
-		orgField = fmt.Sprintf("org:%v", org)
+	for _, org := range orgs {
+		if len(org) != 0 {


The slice returned from getOrganizations can never contain an empty string, right? Do we need the additional check here?

mickmister

Awesome work on this! 🚀 I have a few suggestions and questions to make sure we don't miss anything here. Nice work!

AayushChaudhary0001

This PR has been tested for the following scenario:

Support for multiple orgs
Data updated for multiple orgs in LHS
Notification for all the orgs for which subscription is created

The PR was wroking fine for all the above scenarios, LGTM. Approved.

mickmister · 2024-05-16T19:02:09Z

@AayushChaudhary0001 Did you also test the case of not having an org configured?

AayushChaudhary0001 · 2024-05-20T12:18:22Z

@mickmister Yes, I have tested it with empty field as well for the org support, works fine even if no org is added.

mickmister · 2024-05-21T19:51:41Z

@AayushChaudhary0001 Got it, thank you 👍

mickmister · 2024-05-21T19:51:59Z

@ayusht2810 There are some conflicts to resolve here

mickmister · 2024-06-20T17:40:32Z

@Kshitij-Katiyar @raghavaggarwal2308 I'm curious where this is at currently? On the community server we're currently unable to efficiently collaborate on the mattermost-community projects. Not super urgent, but I don't want this to fall through here. Just checking in

Kshitij-Katiyar · 2024-06-27T09:43:12Z

@mickmister Fixed the conflicts and updated the code with the suggestions. Please have a look.

Kshitij-Katiyar · 2024-06-28T08:20:52Z

@mickmister Fixed the suggestions, please have a look.

AayushChaudhary0001 · 2024-07-04T12:32:15Z

@ayusht2810 I found a issue while testing this PR:
Issue: Data is not getting updated for multiple orgs in LHS

Steps to reproduce:

Connect a github account to Mattermost
Go to plugin settings of Github on Mattermost
Add multiple orgs
Check the data in LHS

Actual - Multiple orgs data is not getting updated in LHS
Expected - Multiple orgs data should get updated in LHS

…ub into MM-420

…le orgs

raghavaggarwal2308 · 2024-07-10T14:39:37Z

@AayushChaudhary0001 The above issue is fixed now. Please re-test.

arush-vashishtha

This PR has been tested for the following scenarios:

Support for multiple orgs
Data updated for multiple orgs in LHS
Notification for all the orgs for which subscription is created

The PR was working fine for all the above mentioned conditions, LGTM. Approved

…ub into MM-420

Co-authored-by: Doug Lauder <wiggin77@warpmail.net>

[MM-420] Add feature to support multiple orgs in plugin settings

18bb85c

ayusht2810 requested review from hanzei and mickmister as code owners April 30, 2024 05:55

ayusht2810 requested a review from raghavaggarwal2308 April 30, 2024 05:55

ayusht2810 self-assigned this Apr 30, 2024

ayusht2810 added the 2: Dev Review Requires review by a core committer label Apr 30, 2024

[MM-420] Fix ci

260672f

hanzei requested changes Apr 30, 2024

View reviewed changes

[MM-420] Review fixes: Update logic of excluding members from flag, u…

a241179

…pdate function definition and minor reveiw fixes

ayusht2810 requested a review from hanzei May 1, 2024 16:57

hanzei reviewed May 2, 2024

View reviewed changes

[MM-420] Update function to get organization and move it to configura…

05426ed

…tion file

ayusht2810 requested a review from hanzei May 7, 2024 12:01

hanzei approved these changes May 7, 2024

View reviewed changes

[MM-420] Update check org function

44ec538

ayusht2810 requested a review from AayushChaudhary0001 May 8, 2024 07:06

[MM-420] Fix lint error

d19986c

mickmister reviewed May 14, 2024

View reviewed changes

AayushChaudhary0001 approved these changes May 16, 2024

View reviewed changes

raghavaggarwal2308 added this to the v2.3.0 milestone Jun 21, 2024

[MM-420]: synced with base and review fixes

6ff2f46

Kshitij-Katiyar requested a review from mickmister June 27, 2024 09:42

mickmister approved these changes Jun 27, 2024

View reviewed changes

Comment thread webapp/src/reducers/index.ts

Comment thread webapp/src/types/github_types.ts

[MM-420]: review fixes

4ae1603

raghavaggarwal2308 requested a review from AayushChaudhary0001 June 28, 2024 09:03

mickmister approved these changes Jun 28, 2024

View reviewed changes

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

hanzei added the 3: QA Review Requires review by a QA tester label Jul 8, 2024

raghavaggarwal2308 modified the milestones: v2.3.0, v2.4.0 Jul 8, 2024

raghavaggarwal2308 added 5 commits July 10, 2024 17:45

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

cbc57f3

…ub into MM-420

Update package-lock file to fix CI errors

343189c

Add default value for organizations

5399bf9

Fix issue reported by QA: RHS not updating properly in case of multip…

440bb97

…le orgs

fix lint error

644af42

arush-vashishtha approved these changes Jul 15, 2024

View reviewed changes

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

166cfc8

…ub into MM-420

AayushChaudhary0001 approved these changes Jul 18, 2024

View reviewed changes

wiggin77 reviewed Jul 26, 2024

View reviewed changes

Comment thread plugin.json Outdated

Review fix: Update help text

281d95b

Co-authored-by: Doug Lauder <wiggin77@warpmail.net>

raghavaggarwal2308 requested a review from wiggin77 July 29, 2024 06:35

wiggin77 approved these changes Jul 29, 2024

View reviewed changes

wiggin77 removed the 2: Dev Review Requires review by a core committer label Jul 29, 2024

raghavaggarwal2308 merged commit d1a6d8d into master Jul 30, 2024

raghavaggarwal2308 deleted the MM-420 branch July 30, 2024 07:52

hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jul 31, 2024

raghavaggarwal2308 mentioned this pull request Oct 23, 2024

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

Merged

Uh oh!

Conversation

ayusht2810 commented Apr 30, 2024

Summary

Ticket Link

What to test?

Checklist

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ayusht2810 commented May 1, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ayusht2810 commented May 7, 2024

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickmister 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!

Uh oh!

Uh oh!

AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

Uh oh!

mickmister commented May 16, 2024

Uh oh!

AayushChaudhary0001 commented May 20, 2024

Uh oh!

mickmister commented May 21, 2024

Uh oh!

mickmister commented May 21, 2024

Uh oh!

mickmister commented Jun 20, 2024

Uh oh!

Kshitij-Katiyar commented Jun 27, 2024

Uh oh!

Uh oh!

Uh oh!

Kshitij-Katiyar commented Jun 28, 2024

Uh oh!

AayushChaudhary0001 commented Jul 4, 2024

Uh oh!