Correctly hide/show quota notifications per project #2222

dtaylor113 · 2017-10-05T18:47:00Z

Addresses issues when switching between projects and viewing and hiding/clearing quota notifications.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1495455#c4

spadgett

My worry is that this doesn't fix the problem for other notifications that don't include the project name in the ID.

@dtaylor113 If you prepend the project name to the uid the notification drawer assigns, does it just work?

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/notifications/notificationDrawerWrapper.js#L187

This might be a one line fix without needing the common changes?

spadgett · 2017-10-05T18:55:38Z

app/scripts/services/resourceAlerts.js

@@ -74,7 +74,8 @@ angular.module("openshiftConsole")
    var setQuotaNotifications = function(quotas, clusterQuotas, projectName) {
      var notifications = QuotaService.getQuotaNotifications(quotas, clusterQuotas, projectName);
      _.each(notifications, function(notification) {
-        if(!NotificationsService.isNotificationPermanentlyHidden(notification)) {
+        // Only add a quota notification if it wasn't specifically set to be hidden by user
+        if(!NotificationsService.isNotificationHidden(notification)) {


I don't think this should be necessary since the drawer tracks what was cleared in session storage. I think the drawer should know whether to show or hide the notification already.

My worry is that this doesn't fix the problem for other notifications that don't include the project name in the ID.

That's why I was working in namespace into the common NotificationsService 😄

@dtaylor113 If you prepend the project name to the uid the notification drawer assigns, does it just work?

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/notifications/notificationDrawerWrapper.js#L187

This might be a one line fix without needing the common changes?

I'll have to test after removing the isNotificationHidden(..) call in resourceAlerts.js. The drawer currently uses EventsService.isCleared() for filtering the APIEvents; I need to add the EventsService.isCleared() call to the notifications callback.

dtaylor113 · 2017-10-05T21:50:59Z

Until namespace is built into NotificationsService.isNotificationVisible(), I'll need to add projectName to the quota notification id. Setting the uid happens after NotificationsService.addNotification(...)

dtaylor113 · 2017-10-06T12:54:54Z

My worry is that this doesn't fix the problem for other notifications that don't include the project name in the ID.

I had to add projectName into the notification id in order for the NotificationsService to process it correctly. I added a conditional for the uid to include the project name if it isn't already in the id.

spadgett · 2017-10-06T13:33:46Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -184,7 +185,7 @@
          // using uid to match API events and have one filed to pass
          // to EventsService for read/cleared, etc
          trackByID: notification.trackByID,
-          uid: id,
+          uid: _.includes(id, project) ? id : project + "/" + id,


I think we should always prepend project rather than trying to check if it's in id. There's no harm and it makes things simpler.

Ok, fixed -thanks

Agree on keeping it simpler. If the id is being created on line 180, can we keep all the id manipulation logic there? Manipulating it in a couple spots might open doors to bugs...

spadgett · 2017-10-06T13:35:49Z

Thanks @dtaylor113. I understand your point now about the memory leak in NotificationsService. I'm good with this change (with one small comment), and I think we should tackle the other problem separately.

spadgett · 2017-10-06T13:43:59Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -99,6 +99,7 @@
        _.each(drawer.notificationGroups, function(group) {
          _.remove(group.notifications, { uid: notification.uid, namespace: notification.namespace });
        });
+        delete notificationsMap[$routeParams.project][notification.uid];


@benjaminapetersen Is this a bug in remove? It seems like the _.each above should take care of this

The notificationsMap is different from the group.notifications. notificationsMap actually generates the group.notifications when switching projects, so it needs to be cleared in the map as well

I understand, thanks.

I would think so as well, looking.

benjaminapetersen · 2017-10-06T13:44:59Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -184,7 +185,7 @@
          // using uid to match API events and have one filed to pass
          // to EventsService for read/cleared, etc
          trackByID: notification.trackByID,


Would it make sense to keep all the id generation/manipulation together on line 180? I'd tend to think it should always be the same value if possible.

Might not be relevant, reviewing again, we have id, trackById, and uid. Bleh.

Either way, I addressed this particular comment. -thanks

spadgett · 2017-10-06T13:47:23Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -172,7 +173,7 @@
      };

      var notificationWatchCallback = function(event, notification) {
-        if(!notification.showInDrawer) {
+        if(!notification.showInDrawer || EventsService.isCleared(notification.id)) {


Shouldn't we be using notification.uid for cleared? We seem to use that in other places.

The notification in this callback doesn't have the uid; the uid is set if it gets past this check.

Yeah, that relates to my comment above. Should always be consistent in the drawer. Think I settled on uid originally because of apiEvents first.

But I don't think we're not checking the right ID.... If we're clearing uid but checking id, isn't this check just wrong?

Ok, addressed this logic issue.

spadgett · 2017-10-06T14:05:53Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+        var project = notification.namespace || $routeParams.project;
+        var id = notification.id ? project + "/" + notification.id : _.uniqueId('notification_') + Date.now();
+
+        if(!notification.showInDrawer || EventsService.isCleared(id)) {


Thanks @dtaylor113, this looks better

spadgett · 2017-10-06T14:07:19Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -99,6 +99,7 @@
        _.each(drawer.notificationGroups, function(group) {
          _.remove(group.notifications, { uid: notification.uid, namespace: notification.namespace });
        });
+        delete notificationsMap[$routeParams.project][notification.uid];


I understand, thanks.

spadgett · 2017-10-06T14:07:33Z

[merge]

openshift-bot · 2017-10-06T14:15:20Z

Evaluated for origin web console merge up to 033fe62

openshift-bot · 2017-10-06T14:15:27Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/320/) (Base Commit: b642a34) (PR Branch Commit: 033fe62)

dtaylor113 · 2017-10-06T15:20:15Z

Follow up issue openshift/origin-web-common#220

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2017

spadgett reviewed Oct 5, 2017

View reviewed changes

dtaylor113 force-pushed the master branch from 0e16fb4 to 5389858 Compare October 5, 2017 21:47

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 5, 2017

dtaylor113 changed the title ~~[DO NOT MERGE] [WIP] Correctly hide/show quota notifications per project~~ Correctly hide/show quota notifications per project Oct 5, 2017

dtaylor113 force-pushed the master branch from 5389858 to a41fd46 Compare October 6, 2017 12:52

spadgett reviewed Oct 6, 2017

View reviewed changes

dtaylor113 force-pushed the master branch from a41fd46 to 03a113e Compare October 6, 2017 13:39

spadgett reviewed Oct 6, 2017

View reviewed changes

benjaminapetersen reviewed Oct 6, 2017

View reviewed changes

spadgett reviewed Oct 6, 2017

View reviewed changes

Correctly hide/show quota notifications per project

033fe62

dtaylor113 force-pushed the master branch from 03a113e to 033fe62 Compare October 6, 2017 14:01

spadgett approved these changes Oct 6, 2017

View reviewed changes

openshift-bot merged commit ec0a953 into openshift:master Oct 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly hide/show quota notifications per project #2222

Correctly hide/show quota notifications per project #2222

dtaylor113 commented Oct 5, 2017 •

edited

Loading

spadgett left a comment

spadgett Oct 5, 2017

dtaylor113 Oct 5, 2017

dtaylor113 commented Oct 5, 2017

dtaylor113 commented Oct 6, 2017 •

edited

Loading

spadgett Oct 6, 2017

dtaylor113 Oct 6, 2017

benjaminapetersen Oct 6, 2017

spadgett commented Oct 6, 2017

spadgett Oct 6, 2017

dtaylor113 Oct 6, 2017

spadgett Oct 6, 2017

benjaminapetersen Oct 6, 2017

benjaminapetersen Oct 6, 2017

benjaminapetersen Oct 6, 2017

dtaylor113 Oct 6, 2017

spadgett Oct 6, 2017

dtaylor113 Oct 6, 2017

benjaminapetersen Oct 6, 2017

spadgett Oct 6, 2017

dtaylor113 Oct 6, 2017

spadgett Oct 6, 2017

spadgett Oct 6, 2017

spadgett commented Oct 6, 2017

openshift-bot commented Oct 6, 2017

openshift-bot commented Oct 6, 2017 •

edited

Loading

dtaylor113 commented Oct 6, 2017

Correctly hide/show quota notifications per project #2222

Correctly hide/show quota notifications per project #2222

Conversation

dtaylor113 commented Oct 5, 2017 • edited Loading

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtaylor113 commented Oct 5, 2017

dtaylor113 commented Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Oct 6, 2017

openshift-bot commented Oct 6, 2017

openshift-bot commented Oct 6, 2017 • edited Loading

dtaylor113 commented Oct 6, 2017

dtaylor113 commented Oct 5, 2017 •

edited

Loading

dtaylor113 commented Oct 6, 2017 •

edited

Loading

openshift-bot commented Oct 6, 2017 •

edited

Loading