Skip to content

[GH-548]:Fixed issue #548 on github 'org lock for username not working' - #658

Merged
mickmister merged 3 commits into
mattermost:masterfrom
Brightscout:MM-548
Apr 11, 2023
Merged

[GH-548]:Fixed issue #548 on github 'org lock for username not working' #658
mickmister merged 3 commits into
mattermost:masterfrom
Brightscout:MM-548

Conversation

@Kshitij-Katiyar

@Kshitij-Katiyar Kshitij-Katiyar commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

Summary

  • When the Organization lock is set to a user's workspace rather than an organization, the create and attach modal will not show any results, so fix that by getting the data for the username as well.

  • Issue #548

@Kshitij-Katiyar
[MI-2872]:Fixed issue mattermost#548 on github 'org lock for username' (
0b09669 
#23)

* [MI-2872]:Fixed issue mattermost#548 on github 'org loack for username'

* [MI-2872]:Fixed lint errors

* [MI-2872]:Fixed review comments

* [MI-2872]:Fixed review commets
@Kshitij-Katiyar
Kshitij-Katiyar requested a review from hanzei as a code owner March 20, 2023 07:42
@mattermost-build

mattermost-build commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

Hello @Kshitij-Katiyar,

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.

@Kshitij-Katiyar Kshitij-Katiyar changed the title [MI-2872]:Fixed issue #548 on github 'org lock for username' [MI-2872]:Fixed issue #548 on github 'org lock for username not working' Mar 20, 2023
@Kshitij-Katiyar Kshitij-Katiyar changed the title [MI-2872]:Fixed issue #548 on github 'org lock for username not working' [GH-548]:Fixed issue #548 on github 'org lock for username not working' Mar 20, 2023
@Kshitij-Katiyar
Kshitij-Katiyar requested a review from m1lt0n March 22, 2023 12:48
@m1lt0n
m1lt0n requested a review from mickmister March 22, 2023 13:05
@hanzei

hanzei commented Mar 23, 2023

Copy link
Copy Markdown
Contributor

@Kshitij-Katiyar Could you please update the issue description using the Fixes #548 syntax so GitHub correctly link the PR to the ticket?

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 23, 2023
hanzei
hanzei requested changes Mar 23, 2023
View reviewed changes
Comment thread server/plugin/api.go Outdated
Comment thread server/plugin/api.go Outdated
Comment thread server/plugin/api.go Outdated
Comment thread server/plugin/api.go
Comment thread server/plugin/api.go Outdated
m1lt0n
m1lt0n suggested changes Mar 23, 2023
View reviewed changes
Comment thread server/plugin/api.go
Comment thread server/plugin/api.go Outdated
mickmister
mickmister reviewed Mar 23, 2023
View reviewed changes

@mickmister mickmister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work @Kshitij-Katiyar 👍 Overall LGTM. I agree with the concerns noted in the reviews above

Comment thread server/plugin/api.go Outdated
@Kshitij-Katiyar Kshitij-Katiyar mentioned this pull request Mar 24, 2023
Merged
@Kshitij-Katiyar
[MI-2929]:Fixed review comments given by mm team for issue mattermost…
0055add 
…#548 (#27)

* [MI-2929]:Fixed review comments given by mm team for issue mattermost#548

* [MI-2929]:Fixed review comment
@codecov-commenter

codecov-commenter commented Mar 24, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage has no change and project coverage change: -0.15 ⚠️

Comparison is base (39a2a70) 15.62% compared to head (0055add) 15.47%.

❗ Current head 0055add differs from pull request most recent head 1363bf7. Consider uploading reports for the commit 1363bf7 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #658      +/-   ##
==========================================
- Coverage   15.62%   15.47%   -0.15%     
==========================================
  Files          15       15              
  Lines        5447     5499      +52     
==========================================
  Hits          851      851              
- Misses       4553     4605      +52     
  Partials       43       43
Impacted Files Coverage Δ
server/plugin/api.go 7.56% <0.00%> (-0.19%) ⬇️

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mickmister
mickmister approved these changes Mar 24, 2023
View reviewed changes
Comment thread server/plugin/api.go Outdated
Comment thread server/plugin/api.go
@m1lt0n
m1lt0n requested review from Willyfrog and removed request for Willyfrog March 27, 2023 07:50
@m1lt0n m1lt0n removed the 2: Dev Review Requires review by a core committer label Mar 28, 2023
@m1lt0n
m1lt0n requested a review from DHaussermann March 28, 2023 12:30
hanzei
hanzei approved these changes Mar 30, 2023
View reviewed changes

@hanzei hanzei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

two nits

Comment thread server/plugin/api.go Outdated
Comment thread server/plugin/api.go Outdated
@Kshitij-Katiyar
Kshitij-Katiyar requested a review from hanzei March 30, 2023 12:03
DHaussermann
DHaussermann approved these changes Apr 5, 2023
View reviewed changes

@DHaussermann DHaussermann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested and passed

  • When locking to a user such as DHaussermann the create option now list repos available as expected
  • Test again the attach but this was already working as expected
  • Regression tested leaving the lock empty no changes found
  • Regression tested locking to an org no changes found

LGTM!

Thanks @Kshitij-Katiyar

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Apr 5, 2023
@mickmister
mickmister merged commit 9012bf4 into mattermost:master Apr 11, 2023
trilopin added a commit that referenced this pull request Apr 17, 2023
@trilopin
Merge branch 'master' into pw-specs
ec8ba8a 
* master:
  [MM-50985] Remove usage of model.AppError (#648)
  [GH-548]:Fixed issue #548 on github 'org lock for username not working'  (#658)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hanzei hanzei hanzei approved these changes
@DHaussermann DHaussermann DHaussermann approved these changes
+2 more reviewers
@m1lt0n m1lt0n m1lt0n approved these changes
@mickmister mickmister mickmister approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

4: Reviews Complete All reviewers have approved the pull request Contributor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Kshitij-Katiyar @mattermost-build @hanzei @codecov-commenter @m1lt0n @mickmister @DHaussermann