Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Improving redirect on guest accounts #3739

Merged
merged 6 commits into from
Oct 8, 2019
Merged

Improving redirect on guest accounts #3739

merged 6 commits into from
Oct 8, 2019

Conversation

jespino
Copy link
Member

@jespino jespino commented Sep 23, 2019

Summary

Fixing certain situation on guest accounts redirects

Ticket Link

MM-17921

@amyblais amyblais added this to the v5.16.0 milestone Sep 23, 2019
@hanzei hanzei added the Work in Progress Not yet ready for review label Sep 24, 2019
@jespino jespino added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Sep 24, 2019
@jespino jespino added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester and removed Work in Progress Not yet ready for review labels Sep 24, 2019
srkgupta
srkgupta approved these changes Sep 24, 2019
View reviewed changes
Copy link
Contributor

@srkgupta srkgupta left a comment

Choose a reason for hiding this comment

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

Changes will be tested once the code is merged into 5.16 release.

@jespino
Can you please confirm if these changes will also be made into mobile app? Will there be a separate PR?

@jespino
Copy link
Member Author

jespino commented Sep 24, 2019

@srkgupta nope, they aren't made, but I'm not 100% sure if they are needed, is this failing in mobile too? This is related to routing and both projects work in a very different way about routing.

@srkgupta
Copy link
Contributor

Thanks @jespino Will recheck this on mobile and raise it as a separate issue if the issue is seen.

@lindalumitchell lindalumitchell added QA Review Done and removed 3: QA Review Requires review by a QA tester labels Sep 25, 2019
mgdelacroix
mgdelacroix approved these changes Sep 26, 2019
View reviewed changes
Copy link
Member

@mgdelacroix mgdelacroix left a comment

Choose a reason for hiding this comment

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

LGTM 👍

mkraft
mkraft reviewed Sep 27, 2019
View reviewed changes
components/channel_layout/channel_controller.jsx
// IE Detection
if (UserAgent.isInternetExplorer() || UserAgent.isEdge()) {
body.classList.add('browser--ie');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding browser--ie (and os--windows and os--mac) from a more global location and have the styling target it with .app_body.browser--ie (or something) so that it doesn't affect existing pages? This just makes the 3rd place that browser--ie is being added and it would be a shame if that number keeps growing. Just wanted to brainstorm it really.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just moving code from one place to another, and is part of cherry-pick PR for 5.16, I'm ok to discuss this as part of another Jira issue, but I think for now, if the code looks good (in the sense of fix the problem and keep the old existing correct behavior), we can merge it and talk about the browser--ie class as a separate thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally fair.

@amyblais amyblais requested a review from mkraft October 3, 2019 23:46
mkraft
mkraft approved these changes Oct 7, 2019
View reviewed changes
components/channel_layout/channel_controller.jsx
// IE Detection
if (UserAgent.isInternetExplorer() || UserAgent.isEdge()) {
body.classList.add('browser--ie');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally fair.

@amyblais amyblais added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Oct 7, 2019
@jespino jespino merged commit 3d2198b into mattermost:master Oct 8, 2019
@jespino jespino deleted the MM-17921 branch October 8, 2019 07:46
@mattermod
Copy link
Contributor

@jespino
Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Fetching origin
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-#3739-upstream-release-5.16-1570520787
Branch 'automated-cherry-pick-of-#3739-upstream-release-5.16-1570520787' set up to track remote branch 'release-5.16' from 'upstream'.
+++ Downloading patch to /tmp/3739.patch (in case you need to do this again)

+++ About to attempt cherry pick of PR. To reattempt:
  $ git am -3 /tmp/3739.patch

Applying: Improving redirect on guest accounts
Using index info to reconstruct a base tree...
M	components/channel_view/channel_view.jsx
Falling back to patching base and 3-way merge...
Auto-merging components/channel_view/channel_view.jsx
CONFLICT (content): Merge conflict in components/channel_view/channel_view.jsx
Patch failed at 0001 Improving redirect on guest accounts
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

+++ Conflicts detected:

UU components/channel_view/channel_view.jsx

+++ Aborting in-progress git am.

+++ Returning you to the master branch and cleaning up.

jespino added a commit to jespino/mattermost-webapp that referenced this pull request Oct 8, 2019
@jespino
Improving redirect on guest accounts (mattermost#3739)
e6a5d63 
* Improving redirect on guest accounts

* Fixing tests

* moving tests to the new correct place
@jespino jespino mentioned this pull request Oct 8, 2019
Merged
jespino added a commit that referenced this pull request Oct 8, 2019
@jespino
Improving redirect on guest accounts (#3739) (#3897)
18f5f0c 
* Improving redirect on guest accounts

* Fixing tests

* moving tests to the new correct place
@jespino jespino added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Oct 8, 2019
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Oct 8, 2019
@srkgupta srkgupta added the Tests/Done Release tests have been written label Oct 11, 2019
Wicked7000 pushed a commit to Wicked7000/mattermost-webapp that referenced this pull request Oct 12, 2019
@jespino @Wicked7000
Improving redirect on guest accounts (mattermost#3739)
b5d40bb 
* Improving redirect on guest accounts

* Fixing tests

* moving tests to the new correct place
sudheerDev added a commit to sudheerDev/mattermost-webapp that referenced this pull request Oct 15, 2019
@sudheerDev
Revert "Improving redirect on guest accounts (mattermost#3739)"
34b1284 
This reverts commit 3d2198b.
brewsterbhg pushed a commit to brewsterbhg/mattermost-webapp that referenced this pull request Nov 11, 2019
@jespino @brewsterbhg
Improving redirect on guest accounts (mattermost#3739)
55fd9f9 
* Improving redirect on guest accounts

* Fixing tests

* moving tests to the new correct place
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mgdelacroix mgdelacroix mgdelacroix approved these changes

@mkraft mkraft mkraft approved these changes

@srkgupta srkgupta srkgupta approved these changes

Assignees

@jespino jespino

Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation QA Review Done Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.16.0
8 participants
@jespino @srkgupta @mattermod @mgdelacroix @mkraft @hanzei @lindalumitchell @amyblais