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.

Sign up for GitHub

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

Jump to bottom

[MM-60676] Fix incorrect callback URL in setup flow #827

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

[MM-60676] Fix incorrect callback URL in setup flow #827

wants to merge 4 commits into from

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Sep 25, 2024

Summary

#811 introduced a regression: The callback URL shown as part of the setup flow was missing the SiteURl.

This PR fixes the issue. On top of that, it unifies the handling of the SiteURL using two helper methods.

Ticket Link

https://mattermost.atlassian.net/browse/MM-60676

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Sep 25, 2024
@hanzei hanzei mentioned this pull request Sep 25, 2024
Merged
@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Sep 25, 2024
wiggin77
wiggin77 approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I like the helper funcs.

@hanzei
Copy link
Contributor Author

hanzei commented Oct 2, 2024

@Kshitij-Katiyar @raghavaggarwal2308 Gentle reminder to review this fix for a regression

raghavaggarwal2308
raghavaggarwal2308 requested changes Oct 3, 2024
View reviewed changes
server/plugin/api.go Outdated Show resolved Hide resolved
server/plugin/plugin.go Outdated Show resolved Hide resolved
@hanzei
Copy link
Contributor Author

hanzei commented Oct 7, 2024

@raghavaggarwal2308 I've updated the PR to ues url.JoinPath and check its error.

raghavaggarwal2308
raghavaggarwal2308 approved these changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 left a comment

Choose a reason for hiding this comment

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

@hanzei LGTM 👍🏽

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Oct 7, 2024
@hanzei hanzei removed the request for review from Kshitij-Katiyar October 7, 2024 19:24
@hanzei
Copy link
Contributor Author

hanzei commented Oct 11, 2024

@AayushChaudhary0001 Gentle reminder to review the PR. I would like to fix the regression that is on master.

@AayushChaudhary0001
Copy link

@hanzei Testing this now on priority

raghavaggarwal2308
raghavaggarwal2308 requested changes Oct 14, 2024
View reviewed changes
server/plugin/utils.go Outdated Show resolved Hide resolved
AayushChaudhary0001
AayushChaudhary0001 approved these changes Oct 15, 2024
View reviewed changes
Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

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

Tested this PR for the following functionalities:-

  • Basic OAuth flow.
  • Connection and disconnection of user's account.

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

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Oct 15, 2024
@hanzei hanzei added this to the v2.4.0 milestone Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wiggin77 wiggin77 wiggin77 approved these changes

@AayushChaudhary0001 AayushChaudhary0001 AayushChaudhary0001 approved these changes

@raghavaggarwal2308 raghavaggarwal2308 Awaiting requested review from raghavaggarwal2308

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@hanzei @AayushChaudhary0001 @wiggin77 @raghavaggarwal2308