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-12205: session expiry notification #866

Merged
merged 2 commits into from
Oct 12, 2018
Merged

MM-12205: session expiry notification #866

merged 2 commits into from
Oct 12, 2018

Conversation

lieut-data
Copy link
Member

Before submitting, please confirm you've

Please provide the following information:

Summary
Detect when a session has expired and display appropriate badging, as per https://docs.google.com/document/d/1yezizH0-3mi4ZJ8nr5vIOiDKv-aBo52fguG__THpDD0/edit#.

Issue link
https://mattermost.atlassian.net/browse/MM-12275

Test Cases

  • Login, then expire session via another session (Account Settings -> Security -> View and Logout of Active Sessions), verify badging

Additional Notes
See mattermost/mattermost-webapp#1751 for corresponding notifications displayed.

@lieut-data lieut-data force-pushed the mm-12205-session-expiry-notification branch from 3732558 to cbe7889 Compare September 24, 2018 22:31
lieut-data
lieut-data commented Sep 24, 2018
View reviewed changes
src/browser/components/MainPage.jsx
@@ -39,6 +39,7 @@ export default class MainPage extends React.Component {

this.state = {
key,
sessionsExpired: new Array(this.props.teams.length),
Copy link
Member Author

Choose a reason for hiding this comment

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

I originally proposed an idea to simplify the interface between the desktop app and the webapp (https://pre-release.mattermost.com/core/pl/qpezbgj1bb898m1q4uk8158x3a), but this proved to be out of scope for the requested feature.

Perhaps something to return to in the future!

src/browser/index.jsx Outdated
if (remote.app.isUnityRunning()) {
remote.app.setBadgeCount(mentionCount);
if (sessionExpired) {
remote.app.setBadgeCount(1);
Copy link
Member Author

Choose a reason for hiding this comment

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

Given the limitation here, I opted to interpret the specs to show a 1 instead of no badge at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this override an existing actual count from other instances?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch! Yes, it probably makes sense to increment mentionCount here instead.

@lieut-data lieut-data mentioned this pull request Sep 24, 2018
Merged
8 tasks
@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 28, 2018

Thanks @lieut-data. mattermost/mattermost-webapp#1751 was merged though, when is it available on pre-release? Sorry due to personal reason, recently it's a little hard to test the master branch on my PC.

@lieut-data
Copy link
Member Author

@yuya-oc, no worries. My webapp changes appear to have gone live this morning on pre-release.

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 9, 2018

Tested on macOS. Looks good to me.

@yuya-oc yuya-oc added the All Platforms null label Oct 9, 2018
@yuya-oc yuya-oc added this to the v4.2.0 milestone Oct 9, 2018
esethna
esethna approved these changes Oct 10, 2018
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

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

Works for me on Windows as well. Thanks @lieut-data!

@lieut-data
Copy link
Member Author

@sudheerDev, do you feel comfortable taking a look at this review?

@sudheerDev
Copy link
Contributor

@lieut-data Sorry about the delay. Will review it tomorrow. I just wanted to read few parts of desktop app code before reviewing this. This is the first thing on my list tomorrow

sudheerDev
sudheerDev approved these changes Oct 12, 2018
View reviewed changes
Copy link
Contributor

@sudheerDev sudheerDev left a comment

Choose a reason for hiding this comment

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

LGTM other than the one question posted.

@lieut-data
Copy link
Member Author

Thanks for the careful review, @sudheerDev!

sudheerDev
sudheerDev approved these changes Oct 12, 2018
View reviewed changes
Copy link
Contributor

@sudheerDev sudheerDev left a comment

Choose a reason for hiding this comment

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

👍

@lieut-data
Copy link
Member Author

@cpanato, did we by chance update the underlying AMI images used for building? This repo is failing with:

...
[3/4] Linking dependencies...
Segmentation fault (core dumped)
> mattermost-desktop@4.2.0-develop build /var/lib/jenkins/jobs/md/jobs/desktop-pulls/branches/PR-866/workspace
> npm-run-all build:*
sh: 1: npm-run-all: not found

@cpanato
Copy link
Contributor

cpanato commented Oct 12, 2018

@lieut-data looks like this build is running on the master jenkins and should not. will check and fix

@cpanato
Copy link
Contributor

cpanato commented Oct 12, 2018

@lieut-data fixed

@lieut-data lieut-data merged commit 4b2684b into master Oct 12, 2018
@lieut-data lieut-data deleted the mm-12205-session-expiry-notification branch October 12, 2018 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esethna esethna esethna approved these changes

@sudheerDev sudheerDev sudheerDev approved these changes

@yuya-oc yuya-oc yuya-oc approved these changes

Assignees
No one assigned
Labels
All Platforms null
Projects
None yet
Milestone
v4.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lieut-data @yuya-oc @sudheerDev @cpanato @esethna