Fix MM-61975 #8459

larkox · 2025-01-08T13:14:19Z

Summary

Fix MM-61975

Ticket Link

Fix https://mattermost.atlassian.net/browse/MM-61975

Release Note

NONE

BenCookie95 · 2025-01-09T17:11:28Z

app/managers/session_manager.ts

@@ -187,7 +187,7 @@ class SessionManager {

    private onSessionExpired = async (serverUrl: string) => {
        this.terminatingSessionUrl.add(serverUrl);
-        await logout(serverUrl, false, false, true);
+        await logout(serverUrl, undefined, true, false, true);


I'm unsure why we call this, we changed skipServerLogout to true and skipEvents is true. We won't do anything in logout apart from return {data: true};? Unless changing skipServerLogout to true was a mistake?

You are completely right. But it makes kind of sense 😅

If the session expired, in theory it expired on the server, so we don't need to contact the server.
And if we are in this part of the code, is because the SESSION_EXPIRED event was triggered, so we don't have to trigger it again.

I guess we can remove the logout completely, yes, but I have mixed feelings because "logically" it makes sense to have it here.

So I guess this call is here in case there are changes to the logout function in future and we forget to add the call back to onSessionExpired? Should we add a comment above this call in case someone else removes it?

abhijit-singh

Looks good to me

larkox · 2025-02-04T16:10:29Z

@rahimrahman @jupenur Friendly reminder to review this

rahimrahman

One small nit, but all good.

rahimrahman · 2025-02-04T16:21:42Z

app/actions/remote/session.ts

+
+            const body = logoutOnAlert ?
+                intl?.formatMessage({id: 'logout.fail.message.forced', defaultMessage: 'We could not log you out of the server. Data may continue to be accessible to this device once the device goes back online.'}) || 'We could not log you out of the server. Data may continue to be accessible to this device once the device goes back online.' :
+                intl?.formatMessage({id: 'logout.fail.message', defaultMessage: 'We could not log you out of the server. If you log out now, data may continue to be accessible to this device once the device goes back online. Do you still want to continue?'}) || 'We could not log you out of the server. If you log out now, data may continue to be accessible to this device once the device goes back online. Do you still want to continue?';


nit: could potentially add to a variable so that you don't have to repeat twice?

assets/base/i18n/en.json

yasserfaraazkhan

Verified if the server is not reachable. We will see the alert when trying to log out.
Verified if the loggin out is cancelled. When server connection is established, we use the app
Verified Logging out of multiple server when device is offline.

larkox · 2025-02-10T10:56:40Z

assets/base/i18n/en.json

+  "logout.fail.cancel": "Cancel",
+  "logout.fail.continue_anyway": "Continue Anyway",
+  "logout.fail.message": "You’re not fully logged out. Some data may continue to be accessible to this device once the device goes back online. What do you want to do?",
+  "logout.fail.message.forced": "We could not log you out of the server. Some data may continue to be accessible to this device once the device goes back online.",
+  "logout.fail.ok": "OK",
+  "logout.fail.title": "Logout not complete",


@cwarnermm I updated the texts. Let me know if I missed anything (apart of the retry button)

thank you!!

larkox · 2025-02-10T10:59:07Z

@abhijit-singh Any opinion on adding a retry button? My concern is that it will clutter the alert with too many buttons and too many options. But 0/5.

abhijit-singh · 2025-02-13T13:22:24Z

@larkox Agreed. I'd hold back on adding a retry button right now. This is an improvement already.

larkox · 2025-02-14T09:35:19Z

@jupenur I re-requested your review since it was not yet approved, and for a final check before merging.

jupenur

LGTM. And Copilot also says it's great 🤖 #tryAI

amyblais · 2025-02-18T07:42:52Z

/cherry-pick release-2.25

mattermost-build · 2025-02-18T07:42:54Z

Cherry pick is scheduled.

amyblais · 2025-02-18T07:42:55Z

/cherry-pick release-2.26

mattermost-build · 2025-02-18T07:42:57Z

Cherry pick is scheduled.

mattermost-build · 2025-02-18T07:42:57Z

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

+++ Updating remotes...
Fetching upstream
hostfile_replace_entries: mkstemp: Read-only file system
update_known_hosts: hostfile_replace_entries failed for /app/.ssh/known_hosts: Read-only file system
From github.com:mattermost/mattermost-mobile
 * [new branch]          build-release-606      -> upstream/build-release-606
   179738038..15a3f491f  edit_profile_cpa       -> upstream/edit_profile_cpa
   ef37432e0..0b1840a7d  feature_schedule_posts -> upstream/feature_schedule_posts
   24e675048..e8d72d8a8  main                   -> upstream/main
 * [new branch]          reportAProblem         -> upstream/reportAProblem
   f00ecc0b0..cd9fc571c  schedule_post_list     -> upstream/schedule_post_list
   014062f78..b9d1ca877  scheduled_post_feature_popup -> upstream/scheduled_post_feature_popup
Fetching upstream
hostfile_replace_entries: mkstemp: Read-only file system
update_known_hosts: hostfile_replace_entries failed for /app/.ssh/known_hosts: Read-only file system
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-mattermost-mobile-#8459-upstream-release-2.25-1739864575
Switched to a new branch 'automated-cherry-pick-of-mattermost-mobile-#8459-upstream-release-2.25-1739864575'
Branch 'automated-cherry-pick-of-mattermost-mobile-#8459-upstream-release-2.25-1739864575' set up to track remote branch 'release-2.25' from 'upstream'.

+++ About to attempt cherry pick of PR #8459 with merge commit 779fe31c3bcb1c8753402e6424219b27a11b2db7.

CONFLICT (modify/delete): app/managers/session_manager.test.ts deleted in HEAD and modified in 779fe31c3 (Fix MM-61975 (#8459)).  Version 779fe31c3 (Fix MM-61975 (#8459)) of app/managers/session_manager.test.ts left in tree.
Auto-merging app/managers/session_manager.ts
Auto-merging assets/base/i18n/en.json
error: could not apply 779fe31c3... Fix MM-61975 (#8459)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
!!! git cherry-pick failed

+++ Aborting in-progress git cherry-pick.

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

* Fix MM-61975 * Add missing strings * Ensure the app gets logged out after not accepting the ToS * Fix i18n and add comment * Fix tests and address feedback * Check for the right value coming from status * Update texts (cherry picked from commit 779fe31)

* Fix MM-61975 * Add missing strings * Ensure the app gets logged out after not accepting the ToS * Fix i18n and add comment * Fix tests and address feedback * Check for the right value coming from status * Update texts (cherry picked from commit 779fe31) Co-authored-by: Daniel Espino García <larkox@gmail.com>

* Fix MM-61975 * Add missing strings * Ensure the app gets logged out after not accepting the ToS * Fix i18n and add comment * Fix tests and address feedback * Check for the right value coming from status * Update texts

Fix MM-61975

45bb57e

larkox added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 8, 2025

larkox requested review from a team and BenCookie95 and removed request for a team January 8, 2025 13:14

mm-cloud-bot added the release-note-none label Jan 8, 2025

larkox requested a review from rahimrahman January 8, 2025 13:14

larkox added the 2: UX Review Requires review by a UX Designer label Jan 8, 2025

larkox requested a review from abhijit-singh January 8, 2025 13:14

larkox added the 3: Security Review Review requested from Security Team label Jan 8, 2025

larkox requested a review from jupenur January 8, 2025 13:15

abhijit-singh added the Build Apps for PR Build the mobile app for iOS and Android to test label Jan 8, 2025

BenCookie95 requested changes Jan 9, 2025

View reviewed changes

abhijit-singh added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Jan 13, 2025

larkox added 2 commits January 17, 2025 09:55

Merge branch 'main' into alertLogout

ce69419

Add missing strings

3dd0a15

larkox added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Jan 17, 2025

larkox added 2 commits January 21, 2025 12:01

Merge branch 'main' into alertLogout

1332315

Ensure the app gets logged out after not accepting the ToS

8ab3bb5

abhijit-singh approved these changes Jan 28, 2025

View reviewed changes

Fix i18n and add comment

29d4046

larkox force-pushed the alertLogout branch from 6024348 to 29d4046 Compare February 4, 2025 16:08

Merge branch 'main' into alertLogout

4d60f6b

larkox requested a review from BenCookie95 February 4, 2025 16:10

rahimrahman approved these changes Feb 4, 2025

View reviewed changes

Fix tests and address feedback

6ddbf13

jupenur reviewed Feb 7, 2025

View reviewed changes

assets/base/i18n/en.json Outdated Show resolved Hide resolved

yasserfaraazkhan added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Feb 8, 2025

yasserfaraazkhan approved these changes Feb 10, 2025

View reviewed changes

larkox added 2 commits February 10, 2025 11:44

Merge branch 'main' into alertLogout

90b978c

Update texts

3be85fa

larkox commented Feb 10, 2025

View reviewed changes

larkox requested a review from jupenur February 14, 2025 09:29

larkox added QA Review Done and removed 3: QA Review Requires review by a QA tester labels Feb 14, 2025

jupenur approved these changes Feb 17, 2025

View reviewed changes

larkox added 4: Reviews Complete All reviewers have approved the pull request and removed 3: Security Review Review requested from Security Team labels Feb 17, 2025

larkox merged commit 779fe31 into main Feb 17, 2025
27 checks passed

larkox deleted the alertLogout branch February 17, 2025 12:45

mattermost-build mentioned this pull request Feb 18, 2025

Automated cherry pick of #8459 #8599

Merged

mattermost-build added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Feb 18, 2025

amyblais added this to the v2.26.0 milestone Feb 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MM-61975 #8459

Fix MM-61975 #8459

larkox commented Jan 8, 2025

BenCookie95 Jan 9, 2025

larkox Jan 17, 2025

BenCookie95 Jan 21, 2025

abhijit-singh left a comment

larkox commented Feb 4, 2025

rahimrahman left a comment

rahimrahman Feb 4, 2025

yasserfaraazkhan left a comment

larkox Feb 10, 2025

cwarnermm Feb 10, 2025

larkox commented Feb 10, 2025

abhijit-singh commented Feb 13, 2025

larkox commented Feb 14, 2025

jupenur left a comment

amyblais commented Feb 18, 2025

mattermost-build commented Feb 18, 2025

amyblais commented Feb 18, 2025

mattermost-build commented Feb 18, 2025

mattermost-build commented Feb 18, 2025

Fix MM-61975 #8459

Fix MM-61975 #8459

Conversation

larkox commented Jan 8, 2025

Summary

Ticket Link

Release Note

BenCookie95 Jan 9, 2025

Choose a reason for hiding this comment

larkox Jan 17, 2025

Choose a reason for hiding this comment

BenCookie95 Jan 21, 2025

Choose a reason for hiding this comment

abhijit-singh left a comment

Choose a reason for hiding this comment

larkox commented Feb 4, 2025

rahimrahman left a comment

Choose a reason for hiding this comment

rahimrahman Feb 4, 2025

Choose a reason for hiding this comment

yasserfaraazkhan left a comment

Choose a reason for hiding this comment

larkox Feb 10, 2025

Choose a reason for hiding this comment

cwarnermm Feb 10, 2025

Choose a reason for hiding this comment

larkox commented Feb 10, 2025

abhijit-singh commented Feb 13, 2025

larkox commented Feb 14, 2025

jupenur left a comment

Choose a reason for hiding this comment

amyblais commented Feb 18, 2025

mattermost-build commented Feb 18, 2025

amyblais commented Feb 18, 2025

mattermost-build commented Feb 18, 2025

mattermost-build commented Feb 18, 2025