Added user settings part 1 - setting SSH key and impersistent blocking toast notifications #1088

bond95 · 2019-08-13T13:01:14Z

Main page for global settings:

Confirmation modal:

Main page for VM settings:

Save confirmation notification:

Error notification:

lwrigh · 2019-08-13T15:21:45Z

It's looking good to me! One comment is that the context selector on the left should be top aligned with the top of the settings card, not the search bar. Here's the alignment I'm think of:

There are also some future edits that we'll have to make after user testing but we can open another PR for those edits.

yodem

It looks great!!

bond95 · 2019-08-27T11:34:16Z

@lwrigh

lwrigh · 2019-08-27T12:29:12Z

It's looking good! User settings usability testing brought up some issues that we'll have to make updates to. I think it would be better to hold off on merging it before we made those updates. @sgratch @bond95 @sjd78 @yodem What do you guys think?

sjd78 · 2019-09-07T04:02:06Z

@bond95 - CI is failing on an i18n test that is checking to make sure all keys in the translations exist as English keys. In other words, existing keys were removed or renamed. Easiest fix is to remove the offending keys from the translation file. Or if the keys are a rename, and you're feeling nice, adjust the translation key names.

lwrigh · 2019-10-16T06:08:43Z

@bond95 Is it ready for updated screenshots yet? Just saw that it's waiting for my review but I'm just waiting for updated screenshots of it. We can also do a demo of it so we can review the behavior when it's ready.

bond95 · 2019-10-18T12:41:20Z

@lwrigh
Moved global settings button to main panel:

Global user settings:

Saved settings:

VM settings:

Link to settings in console page:

Multiselect:

Multiselect settings:

Confirmation dialog:

lwrigh · 2019-10-21T01:39:28Z

@bond95 Awesome this is looking great! My only comment is that I would make the dropdown and text fields a bit less wide. They could probably be half the width.

bond95 · 2019-10-21T14:21:25Z

@lwrigh updated width of settings:

WDYT?

lwrigh · 2019-10-22T01:55:41Z

@lwrigh updated width of settings:

WDYT?

The fields are still a bit too wide, I would recommend it be half the width so it looks like something like this:

bond95 · 2019-10-22T12:23:19Z

@lwrigh

lwrigh · 2019-10-23T01:18:18Z

@lwrigh

The dropdowns look good! Ah sorry if I wasn't communicating clearly, the text for the checkboxes should be on all one line and should only go to two lines when it gets closer to the edge of the card. Generally when it comes to sentences they should try to extend the width of the container but with components like the dropdowns the width is usually determined by the content.

bond95 · 2019-10-23T11:56:45Z

@lwrigh fixed:

rszwajko · 2020-12-01T13:23:55Z

Expected result for "do not disturb" enabled mode:
notifications shouldn't appear within the notification drawer

They seem to appear in Web Admin so I would keep them here too.

Expected result for "do not disturb" disabled mode:
notifications shouldn't be dismissed from the notification drawer even after refresh.

Fixed. Instead of auto-removing we now acknowledge old entries.

sgratch · 2020-12-01T13:43:19Z

@rszwajko

1. The email field within "Account Settings" dialog is always empty even if email is set for the user.

OK. I'll remove it from this section.

Why removing and not fixing? Isn't it a bug?

2. Please update PR's name and description to reflect current content of: user settings for SSH key and local not persistent notifications disabling only.
Looking from the end user perspective I agree but this patch still contains a lot of framework code that will be used by server-side user settings (coming soon).

I know but we can't merge this PR (with a very very long conversation list :)) without specifying exactlly what's merged and what is the context of this PR. No one will understand/remember that from reading all long conversations.
You can change that to something like "added user setting 1st phase- SSH key and non persistent notification disabling only (includes base framework code for future/backend code" or something like that. Either on title or as the 1st comment.

rszwajko · 2020-12-02T10:43:22Z

The email field within "Account Settings" dialog is always empty even if email is set for the user.

OK. I'll remove it from this section.

Why removing and not fixing? Isn't it a bug?

User email is fetched using /users/<id> REST endpoint which is currently not working for standard users.

Please update PR's name and description to reflect current content of: user settings for SSH key and local not persistent notifications disabling only.

Done. Please check the current PR title.

Notifications settings: please consider adding to the tooltip "Notification settings applied here affect all notifications" a clarification that it includes affecting all VMs and Pools notifications. It seems obvious but maybe worth mentioning.

Done.

sgratch · 2020-12-03T20:32:45Z

@rszwajko

Expected result for "do not disturb" disabled mode:
notifications shouldn't be dismissed from the notification drawer even after refresh.

Fixed. Instead of auto-removing we now acknowledge old entries.

+1
Note that there is a separate issue, not related to this PR, which prevents from keeping old entries: #1338

Notifications settings: please consider adding to the tooltip "Notification settings applied here affect all notifications" a clarification that it includes affecting all VMs and Pools notifications. It seems obvious but maybe worth mentioning.

Done.

What I meant on my original comment is to emphasize the fact that all vms/pools are affected since you can't filter by vms/pools for now. So maybe just consider adding "all": ...(including all vms and pools notifications).

I had trouble finding the right phrase. Session duration is a shorter form of browser session duration (which seemed too long for me). The notifications settings are saved only in Redux store (not in local storage) so every page reload (new browser session) clears them. Server side session(login/logout) is different concept and is not impacted by page reload.
"until next login or hard refresh"
I'm not sure if hard refresh is clear enough - this implies that we have also a soft reset somewhere :)
until next login is self-explanatory but only partially correct (usually login involves page reload).

The problem is that session for web-ui terminology is referring to user session. Check for example the config value: https://zipur.ca/knowledgebase/stop-engine-webui-session-time-logging-out/
I still think that using "session duration" is very confusing so I suggest adding a tooltip that explains this 4th option. WDYT?

    Please update PR's name and description to reflect current content of: user settings for SSH key and local not persistent notifications disabling only.

Done. Please check the current PR title.

Great, thanks! Can you please just add "impersistent" to the "blocking toast notifications" part on title.

=====================
Other than that all LGTM.
If #1334 is not reviewed yet tomorrow then please rebase without it for now and we'll handle that later on.

rszwajko · 2020-12-04T15:51:15Z

@sgratch

consider adding "all": ...(including all vms and pools notifications).

Done. Additionally, I've extended the tooltip to explain that the settings are not persisted. This is true for the entire section so section tooltip seems the best place

I still think that using "session duration" is very confusing so I suggest adding a tooltip that explains this 4th option.

I've changed this label to "until next page reload". This seems the most self explanatory term. It does sounds strange when you read it together with the dropdown label ("Do not disturb for until next page reload") but as confirmed with @sjd78 it's not that bad. Web Admin uses similar wording here ("until next Log in").

sgratch · 2020-12-06T16:46:59Z

@rszwajko
All LGTM now and ready to be finally merged except:

If #1334 is not reviewed yet tomorrow then please rebase without it for now and we'll handle that later on.

I would like a couple of eyes to review this (specifically Scott) before merging this part.

sjd78

Changes look good to me.

rszwajko · 2020-12-07T11:39:39Z

Rebased on top of #1334 (version with improved logging)

CounterAlert: 1. onDismiss prop - callback fired when alert is closed 2. children prop - rendered below title

Summary: 1. Remove existing Options dialog a) move SSH Key to Account Settings b) continue using existing REST endpoint: users/${id}/sshpublickeys 2. support following (local) settings via Redux-store: a) showNotifications (do not disturb) b) notificationSnoozeDuration (do not disturb for) 3. add typing for user settings related entities 4. in Settings use data-driven 3-way diff for change detection. Change is detected by comparing current content of Redux store with user changes(draft values) and base values(store content before at the beginning of user settings updates)

Allow user to block system notifications via Account Settings: 1. each change to the snooze duration resets the snooze timer 2. notification settings are not persisted - page reload clears them. 3. only displaying is blocked - logs continue to be accumulated in the store (userMessages -> records)

rszwajko · 2020-12-07T12:51:58Z

Rebased on top of #1334 (after merge GH reported conflicts in this PR)

sgratch

LGTM

isaranova · 2021-04-16T12:22:47Z

LGTM

bond95 requested review from sjd78, yodem, lwrigh and thaorell August 13, 2019 13:01

yodem previously approved these changes Aug 14, 2019

View reviewed changes

bond95 force-pushed the user-settings branch from 977444c to d6d363a Compare August 27, 2019 11:33

This was referenced Aug 29, 2019

Setting Connect automatically to console removes this setting for other user #685

Closed

VM properties missing in VM portal #985

Closed

bond95 force-pushed the user-settings branch from d6d363a to ca78160 Compare August 29, 2019 12:50

bond95 force-pushed the user-settings branch from 9bea940 to e80a415 Compare October 7, 2019 11:53

bond95 force-pushed the user-settings branch from c744cab to 7e10e6b Compare October 18, 2019 12:40

bond95 force-pushed the user-settings branch 3 times, most recently from 6701940 to f606dc1 Compare October 21, 2019 14:17

bond95 force-pushed the user-settings branch from f606dc1 to c5f1763 Compare October 22, 2019 12:21

bond95 force-pushed the user-settings branch from c5f1763 to 247505a Compare October 23, 2019 11:56

bond95 force-pushed the user-settings branch from 247505a to 2c1a39f Compare October 23, 2019 12:01

lwrigh approved these changes Oct 24, 2019

View reviewed changes

rszwajko force-pushed the user-settings branch from ac910e4 to 098dba8 Compare December 2, 2020 10:36

rszwajko changed the title ~~Added user settings~~ Added user settings part 1 - setting SSH key and blocking toast notifications Dec 2, 2020

rszwajko changed the title ~~Added user settings part 1 - setting SSH key and blocking toast notifications~~ Added user settings part 1 - setting SSH key and impersistent blocking toast notifications Dec 4, 2020

rszwajko force-pushed the user-settings branch from 098dba8 to 13e9bc2 Compare December 4, 2020 15:36

rszwajko requested review from sgratch and sjd78 December 4, 2020 15:37

sjd78 approved these changes Dec 7, 2020

View reviewed changes

rszwajko force-pushed the user-settings branch from 13e9bc2 to 243abea Compare December 7, 2020 11:34

rszwajko added 3 commits December 7, 2020 13:42

Extend CounterAlert and SelectBox components

99844ec

CounterAlert: 1. onDismiss prop - callback fired when alert is closed 2. children prop - rendered below title

rszwajko force-pushed the user-settings branch from 243abea to ae9a27c Compare December 7, 2020 12:50

sgratch approved these changes Dec 7, 2020

View reviewed changes

sgratch added the Status: ON_QA status ON_QA (currently being tested) label Dec 7, 2020

sgratch merged commit 9d0e524 into oVirt:master Dec 7, 2020

rszwajko mentioned this pull request Feb 25, 2021

Change locale and display email via Account Settings #1372

Merged

rszwajko mentioned this pull request Apr 6, 2021

Adding confirmation modal on saving user settings changes #1401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added user settings part 1 - setting SSH key and impersistent blocking toast notifications #1088

Added user settings part 1 - setting SSH key and impersistent blocking toast notifications #1088

bond95 commented Aug 13, 2019

lwrigh commented Aug 13, 2019

yodem left a comment

bond95 commented Aug 27, 2019

lwrigh commented Aug 27, 2019

sjd78 commented Sep 7, 2019

lwrigh commented Oct 16, 2019

bond95 commented Oct 18, 2019

lwrigh commented Oct 21, 2019

bond95 commented Oct 21, 2019

lwrigh commented Oct 22, 2019

bond95 commented Oct 22, 2019

lwrigh commented Oct 23, 2019

bond95 commented Oct 23, 2019

rszwajko commented Dec 1, 2020

sgratch commented Dec 1, 2020

rszwajko commented Dec 2, 2020

sgratch commented Dec 3, 2020

rszwajko commented Dec 4, 2020

sgratch commented Dec 6, 2020 •

edited

Loading

sjd78 left a comment

rszwajko commented Dec 7, 2020

rszwajko commented Dec 7, 2020

sgratch left a comment

isaranova commented Apr 16, 2021

Added user settings part 1 - setting SSH key and impersistent blocking toast notifications #1088

Added user settings part 1 - setting SSH key and impersistent blocking toast notifications #1088

Conversation

bond95 commented Aug 13, 2019

lwrigh commented Aug 13, 2019

yodem left a comment

Choose a reason for hiding this comment

bond95 commented Aug 27, 2019

lwrigh commented Aug 27, 2019

sjd78 commented Sep 7, 2019

lwrigh commented Oct 16, 2019

bond95 commented Oct 18, 2019

lwrigh commented Oct 21, 2019

bond95 commented Oct 21, 2019

lwrigh commented Oct 22, 2019

bond95 commented Oct 22, 2019

lwrigh commented Oct 23, 2019

bond95 commented Oct 23, 2019

rszwajko commented Dec 1, 2020

sgratch commented Dec 1, 2020

rszwajko commented Dec 2, 2020

sgratch commented Dec 3, 2020

rszwajko commented Dec 4, 2020

sgratch commented Dec 6, 2020 • edited Loading

sjd78 left a comment

Choose a reason for hiding this comment

rszwajko commented Dec 7, 2020

rszwajko commented Dec 7, 2020

sgratch left a comment

Choose a reason for hiding this comment

isaranova commented Apr 16, 2021

sgratch commented Dec 6, 2020 •

edited

Loading