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.

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

[MM-59483] Remove legacy preload and custom login code #3174

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Conversation

devinbinnie
Copy link
Member

Summary

This PR removes code in the Desktop App that was left to support older versions of the Mattermost Server/Web App:

  • Removed the legacy preload listeners using window.postMessage, needed for server versions <v9.4
  • Removed the custom login code that allows the app to go out to another site and login with the app, needed for versions <v9.2.

What this does mean is that as of this change, Mattermost servers running versions under v9.4 will now have a reduced experience, and may or may not function at all. We will be dropping support for those server versions in the Desktop App officially as of v5.11.

This is being done to both reduce complication, improve security (in the case of custom logins) and potentially improve performance (in the case of the preload script changes)

Ticket Link

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

Remove legacy code for older unsupported Mattermost servers

@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 21, 2024
@devinbinnie devinbinnie added this to the v5.11.0 milestone Oct 21, 2024
@devinbinnie devinbinnie requested review from yasserfaraazkhan, a team, gabrieljackson and calebroseland and removed request for a team October 21, 2024 20:01
@devinbinnie
Copy link
Member Author

@yasserfaraazkhan We can test this whenever, but since we're removing support for older versions, we should double check that the versions we support are all working well :)
I ran E2E tests on my local machine and everything went okay - but a bit more thorough testing seems applicable here.

@yasserfaraazkhan yasserfaraazkhan added the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Oct 21, 2024
Copy link

Here are the test results below:

Test Summary for Linux on commit 3b51570

New failed tests found on Linux:

  • menu/view MM-T816 Toggle Full Screen in the Menu Bar

Test Summary for macOS on commit 3b51570

All stable tests passed on macOS.

@github-actions github-actions bot removed the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Oct 21, 2024
Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan 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 with servers

  • v9.5 ✅
  • v9.4 ✅
  • v9.2 & v9.0 the login flow was stuck and in logs we can see error [TAB_MESSAGING] Cannot reach server: Error: net::ERR_CONNECTION_REFUSED

@devinbinnie devinbinnie removed the 3: QA Review Requires review by a QA tester label Oct 22, 2024
Copy link

@gabrieljackson gabrieljackson left a comment

Choose a reason for hiding this comment

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

Nice clean up. I am assuming the failing Electron Playwright Tests is expected or being worked on separately?

@devinbinnie devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Oct 25, 2024
@devinbinnie devinbinnie merged commit 6d37cc2 into master Oct 25, 2024
27 of 29 checks passed
@devinbinnie devinbinnie deleted the MM-59483 branch October 25, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants