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

Fixing leave channel when another window is open #4270

Merged
merged 2 commits into from
Nov 22, 2019
Merged

Fixing leave channel when another window is open #4270

merged 2 commits into from
Nov 22, 2019

Conversation

jespino
Copy link
Member

@jespino jespino commented Nov 19, 2019

Summary

Fixing leave channel when another window is open

Ticket Link

MM-20206

@jespino jespino requested a review from a team November 19, 2019 20:17
@ghost ghost requested review from hanzei and saturninoabril and removed request for a team November 19, 2019 20:17
@jespino jespino requested review from DHaussermann and removed request for saturninoabril and hanzei November 19, 2019 20:17
@jespino jespino added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Nov 19, 2019
@jespino jespino added this to the v5.17.0 milestone Nov 19, 2019
@jespino jespino added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Nov 19, 2019
@DHaussermann DHaussermann added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Nov 20, 2019
@mattermod
Copy link
Contributor

Mattermost test server updated with git commit 9f6326f0e443f1d7628df9a7d6f0fbe84062eb68.

Access here: https://mattermost-webapp-pr-4270.test.mattermost.cloud

@jespino jespino requested a review from enahum November 20, 2019 14:43
enahum
enahum suggested changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

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

code looks good, what about tests for this?

@jespino
Copy link
Member Author

jespino commented Nov 20, 2019

Testing this would be super complicated because is related to async execution, I don't really know what is the other concurrent part interfering here, and I could try a cypress test, but if I'm not wrong, cypress doesn't allow multi window/tab testing.

@jespino jespino requested a review from enahum November 20, 2019 14:48
@enahum
Copy link
Contributor

enahum commented Nov 20, 2019

Ok I'll let QA decide on that

@DHaussermann
Copy link

Tested and passed. This issue is resolved.

  • Tested viewing a channel from browser and desktop while the user leaves
  • Regression tested behavior on RN
    No issues found.

@saturninoabril curious if this can be done with Cypress easily enough? Would need to validate proper functionality while the user leaves the channel.

@saturninoabril
Copy link
Member

Short answer is no; it's a Cypress limitation like Jesus mentioned.
There are cases where we can imitate multi-client scenario like when the server is only relaying info from one client to another (e.g. one is posting a message [do via API] and the other observes it [on browser]) -- seems like 2 clients but actually observing on one browser only. The issue here is the test requires client to be live at the same time where Cypress don't support it.

@jespino
Copy link
Member Author

jespino commented Nov 20, 2019

Maybe this is a good option for rain forest.

@DHaussermann
Copy link

Since the bug does repro using a 2nd incognito browser in a new session, we should be able to cover it with RainForest I have queued the test for RF.

DHaussermann
DHaussermann approved these changes Nov 20, 2019
View reviewed changes
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Thanks @jespino
LGTM!

@jespino jespino requested a review from hanzei November 20, 2019 15:48
@DHaussermann DHaussermann removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Nov 20, 2019
@mattermod
Copy link
Contributor

Test server destroyed

@ghost ghost requested review from grundleborg and removed request for a team November 20, 2019 22:23
@hanzei hanzei removed the request for review from grundleborg November 20, 2019 22:23
@gigawhitlocks gigawhitlocks removed their request for review November 20, 2019 22:23
@hanzei hanzei requested a review from a team November 20, 2019 22:23
@ghost ghost requested review from jwilander and sudheerDev and removed request for a team November 20, 2019 22:23
@hanzei hanzei removed the request for review from jwilander November 20, 2019 22:23
@hanzei
Copy link
Contributor

hanzei commented Nov 20, 2019

@jespino I don't feel comfortable reviewing this code and requested a review from someone else.

@mgdelacroix mgdelacroix self-requested a review November 22, 2019 11:45
mgdelacroix
mgdelacroix approved these changes Nov 22, 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 👍

@jespino jespino removed the request for review from sudheerDev November 22, 2019 12:37
@jespino jespino merged commit edcac8b into mattermost:master Nov 22, 2019
@jespino jespino deleted the MM-20206 branch November 22, 2019 12:37
@mattermost-build mattermost-build mentioned this pull request Nov 22, 2019
Merged
@mattermod mattermod 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 Nov 22, 2019
@jespino jespino added 4: Reviews Complete All reviewers have approved the pull request CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed 2: Dev Review Requires review by a core commiter CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone labels Nov 22, 2019
jespino added a commit that referenced this pull request Nov 22, 2019
@jespino
Fixing leave channel when another window is open (#4270)
2412b74 
* Fixing leave channel when another window is open

* Fixing tests and addressing PR review comments
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Nov 22, 2019
jespino pushed a commit that referenced this pull request Nov 24, 2019
@mattermost-build @jespino
Automated cherry pick of #4270 (#4318)
a64c78e 
* Fixing leave channel when another window is open

* Fixing tests and addressing PR review comments
@amyblais
Copy link
Member

@jespino Can you confirm this is cherry-picked to 5.17?

@jespino
Copy link
Member Author

jespino commented Nov 25, 2019

@amyblais yes, I cherry-picked that yesterday.

@hanzei hanzei 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 Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mgdelacroix mgdelacroix mgdelacroix approved these changes

@enahum enahum enahum approved these changes

@DHaussermann DHaussermann DHaussermann approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation
Projects
None yet
Milestone
v5.17.0
8 participants
@jespino @mattermod @enahum @DHaussermann @saturninoabril @hanzei @amyblais @mgdelacroix