Quota Notifications #2116

dtaylor113 · 2017-09-19T14:19:21Z

Push quota notifications to notification drawer:

dtaylor113 · 2017-09-19T14:44:39Z

app/scripts/services/resourceAlerts.js

+    var setQuotaNotifications = function(quotas, clusterQuotas, projectName) {
+      var notifications = QuotaService.getQuotaNotifications(quotas, clusterQuotas, projectName);
+      _.each(notifications, function(notification) {
+        // NotificationsService.restorePermanentlyHiddenNotification(notification.id, projectName);


Oops...debug op...I'll remove before final merge

spadgett · 2017-09-19T14:28:57Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -259,6 +260,26 @@
          onLinkClick: function(link) {
            link.onClick();
            drawer.drawerHidden = true;
+          },
+          handleAction: function(notification, action, index, group) {


This logic shouldn't be in the drawer. Whoever adds the actions should be responsible for handling them.

spadgett · 2017-09-19T14:29:36Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+            switch (action.name) {
+              case 'View Quotas':
+                $location.url("/project/" + $routeParams.project + "/quota");
+                drawer.drawerHidden = true;


The link in the view should handle this for all actions on click.

spadgett · 2017-09-19T14:33:25Z

app/scripts/services/quota.js

@@ -254,6 +257,84 @@ angular.module("openshiftConsole")
      });
    };

+    var OBJECT_COUNT_QUOTAS = [


This list is going to constantly be out of date as things are added upstream. Is it really needed? We should check whatever is declared in the quota.

spadgett · 2017-09-19T14:38:12Z

app/scripts/services/quota.js

+    var getNotificaitonMessage = function(used, usedValue, hard, hardValue, quotaKey) {
+      var msgPrefix = "Your project is " + (hardValue < usedValue ? 'over' : 'at') + " quota. ";
+      var msg;
+      if (_.includes(OBJECT_COUNT_QUOTAS, quotaKey)) {


Let's remove OBJECT_COUNT_QUOTAS and add a COMPUTE_RESOURCE_QUOTAS, then check if quotaKey is in COMPUTE_RESOURCE_QUOTAS instead. The compute resource list is smaller, so will be easier to maintain. I'd also rather only call usageWithUnits if we are sure it as a compute resource, not as a fallback. Otherwise that will be called for unrecognized types.

spadgett · 2017-09-19T15:27:02Z

app/scripts/services/quota.js

+      if (_.includes(OBJECT_COUNT_QUOTAS, quotaKey)) {
+        msg = msgPrefix + "It is using " + usedValue + " of " + hardValue + " " + humanizeQuotaResource(quotaKey, true) + ".";
+      } else {
+        msg = msgPrefix + "It is using " + (usedValue / hardValue * 100) + "% of " + usageWithUnits(hard, quotaKey) + " " + humanizeQuotaResource(quotaKey, true) + ".";


We should round that percentage to avoid things like repeating decimals. I suggest using the percent filter:

origin-web-console/app/scripts/filters/util.js

Lines 461 to 470 in c6ac85f

.filter('percent', function() {

// Takes a number like 0.33 and returns "33%". `precision` is the optional

// number of digits to appear after the decimal point.

return function(value, precision) {

if (value === null || value === undefined) {

return value;

}

return _.round(Number(value) * 100, precision) + "%";

};

})

spadgett · 2017-09-19T15:31:32Z

app/scripts/services/quota.js

+              message: getNotificaitonMessage(used, usedValue, hard, hardValue, quotaKey),
+              skipToast: true,
+              showInDrawer: true,
+              actions: [


Move the action on click handlers from the notification drawer to here. You can pass it as an onClick property in the action.

spadgett · 2017-09-19T15:34:04Z

app/views/directives/notifications/notification-body.html

@@ -35,7 +36,7 @@
          href=""
          class="secondary-action"
          title="{{action.title}}"
-          ng-click="$ctrl.customScope.handleAction(notification, action)">
+          ng-click="$ctrl.customScope.handleAction(notification, action, $parent.$parent.$index, notificationGroup)">


I'd change this to simply action.onClick() if you take my suggestion above.

spadgett · 2017-09-19T15:34:35Z

app/views/directives/notifications/notification-body.html

@@ -71,7 +72,7 @@
        </span>

        <span ng-if="!(notification.event.involvedObject)">
-          {{notification.message}}
+          <span ng-bind-html="notification.message"></span>


Can't assume that the message is HTML. We need a flag that says use HTML for the message. Otherwise plain text messages will not properly be escaped.

Ok, should I check the message for HTML tags, or if I detect that a custom message is being used, is that enough to process it as HTML?

No way to reliably detect if it has markup. I would document that the custom quota messages are markup. Then add another a flag like notification.isHTML that tells the drawer how to render it.

dtaylor113 · 2017-09-19T18:25:46Z

Hi @spadgett, I believe I have addressed all of your review issues.
Thanks,
Dave

spadgett · 2017-09-19T18:39:29Z

app/scripts/constants.js

+  QUOTA_NOTIFICATION_MESSAGE: {
+    // Example quota messages to show in notification drawer
+    // "pods": "Upgrade to <a href='http://www.google.com'>OpenShift Pro</a> if you need additional resources.",
+    // "limits.memory": "Upgrade to <a href='http://www.google.com'>OpenShift Online Pro</a> if you need additional resources."


nit: can we change the URLs to openshift.com?

spadgett · 2017-09-19T18:40:34Z

app/scripts/filters/util.js

@@ -515,4 +515,13 @@ angular.module('openshiftConsole')
    return function(feature) {
      return _.get(Constants, ['ENABLE_TECH_PREVIEW_FEATURE', feature], false);
    };
+  })
+  .filter('hasHTML', function() {


Let's not try to test for HTML tags because this can have false positives. I would remove this filter.

spadgett · 2017-09-19T18:43:30Z

app/scripts/services/quota.js

+
+      if (Constants.QUOTA_NOTIFICATION_MESSAGE && Constants.QUOTA_NOTIFICATION_MESSAGE[quotaKey]) {
+        msg += " " + Constants.QUOTA_NOTIFICATION_MESSAGE[quotaKey];
+      }


Always generate an HTML message here and set isHTML: true below.

msg = _.escape(msg); if (Constants.QUOTA_NOTIFICATION_MESSAGE && Constants.QUOTA_NOTIFICATION_MESSAGE[quotaKey]) { msg += " " + Constants.QUOTA_NOTIFICATION_MESSAGE[quotaKey]; } return msg;

spadgett · 2017-09-19T18:43:45Z

app/scripts/services/quota.js

+              namespace: projectName,
+              type: (hardValue < usedValue ? 'warning' : 'info'),
+              message: msg,
+              isHTML: containsHTML,


isHTML: true with the suggested changes above.

spadgett · 2017-09-19T18:45:53Z

app/scripts/services/quota.js

+                    NotificationsService.permanentlyHideNotification(notification.uid, notification.namespace);
+                    EventsService.markCleared(notification.uid);
+                    group.notifications.splice(index, 1);
+                    drawer.customScope.countUnreadNotifications();


I'd change these onClick handlers to return true if they should clear the notification and then move these two lines back to the drawer. The onClick handler shouldn't need to take any parameters (you have a reference to the notification already through the closure).

Notification drawer wrapper can check the onClick return value and then clear the notification if it's true.

action.onClick isn't being called from the notificationDrawerWrapper.js, it is being called from the included notification-body.html, here.
In quota.js, I could throw $rootScope.$emit('NotificationDrawerWrapper.clear') and also pass along the notification.id/uid.

action.onClick isn't being called from the notificationDrawerWrapper.js, it is being called from the included notification-body.html

Right, I understand. I'm proposing changing that to follow this basic pattern:

https://github.com/openshift/origin-web-console/blob/master/app/views/_alerts.html#L24

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/alerts.js#L34-L42

spadgett · 2017-09-19T18:58:26Z

app/scripts/services/quota.js

+                  title: 'View project quotas',
+                  onClick: function(notification, action, index, group, drawer) {
+                    $location.url("/project/" + $routeParams.project + "/quota");
+                    drawer.drawerHidden = true;


Can we use something like

$rootScope.$emit('NotificationDrawerWrapper.hide');

to close the drawer? We're already listening for a toggle message in the drawer, so it should not be too tough to add another. I'd rather not pass the drawer as a parameter.

dtaylor113 · 2017-09-19T19:16:18Z

The onClick handler shouldn't need to take any parameters (you have a reference to the notification already through the closure).

Hi @spadgett, sorry, you lost me here, can you please elaborate? -thanks

spadgett

@dtaylor113 I don't need you need a reference to notification at all in the onClick callback. See my comment below.

spadgett · 2017-09-19T19:21:12Z

app/scripts/services/quota.js

+                  name: "Don't Show Me Again",
+                  title: 'Permenantly hide this notificaiton until quota limit changes',
+                  onClick: function(notification, action, index, group, drawer) {
+                    NotificationsService.permanentlyHideNotification(notification.uid, notification.namespace);


I don't thinknotification.uid is right. Where is uid getting set?

It should be something like

var notificationID = 'openshift/hide-quota-warning/' + projectName + '/' + quotaKey;

notification.uid is being set here

OK, I see. We should be scoping to a project. So maybe just above at line 313:

var notificationID = 'quota-warning/' + projectName + '/' + quotaKey;

Then we can use that value here and don't need a reference to the notification itself.

By the way, I don't think the NotificationDrawerWrapper is setting a uid anymore.

common's NotificationService already has an API for permanently hiding a notification. It takes an id and a 'namespace', which in this case is 'projectName'. So I don't think 'projectName' should be in the notificationID.

Sorry, I see. That's fine then.

I think the point that the notification doesn't need to be passed back to the onClick handlers stands, though. We already have quotaKey and projectName.

By the way, I don't think the NotificationDrawerWrapper is setting a uid anymore.

I still see it being set in Ben's PR here

You're right. I don't think that's needed in Ben's PR -- we shouldn't need 3 separate IDs -- but I'll leave well enough alone since it's in the merge queue.

3 IDs, wheeeee... 😄
Yeah I agree, oops on my part, I prob should have dropped one after adding trackByID.

dtaylor113 · 2017-09-19T20:29:38Z

Hi @spadgett, updated with latest rounds of changes. I believe this is more inline with the separation of services. Quotas and the NotificationDrawer now communicate via events and hand off responsibilities appropriately.

spadgett

Thanks @dtaylor113, looks good. Just a few small comments.

Ben's PR has merged, so you should be able to rebase.

spadgett · 2017-09-20T10:40:36Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -217,6 +227,7 @@
        hasUnread: false,
        showClearAll: true,
        showMarkAllRead: true,
+        removeNotificationFromGroup: removeNotificationFromGroup,


Shouldn't need to add to drawer if it's not used in the view.

spadgett · 2017-09-20T10:44:53Z

app/scripts/services/quota.js

+      "limits.memory"
+    ];
+
+    var getNotificaitonMessage = function(used, usedValue, hard, hardValue, quotaKey) {


Worth a comment that this function returns HTML markup, not plain text.

spadgett · 2017-09-20T10:47:03Z

app/scripts/services/quota.js

+      msg = _.escape(msg);
+
+      if (Constants.QUOTA_NOTIFICATION_MESSAGE && Constants.QUOTA_NOTIFICATION_MESSAGE[quotaKey]) {
+        msg += " " + Constants.QUOTA_NOTIFICATION_MESSAGE[quotaKey];


I'd comment here as well that the custom message is markup. Then it's clear why you don't need to call _.escape.

The custom message could just be a string and not contain HTML, right? Not sure what makes the custom message 'markup'? Is it escaped by being a Constant?

Sorry if I wasn't clear. I'm saying to add a comment like

// QUOTA_NOTICIATION_MESSAGE can contain HTML and shouldn't be escaped.

I don't want someone to add an _.escape call later thinking it was an oversight.

ah..ok..will do

Added comment

spadgett · 2017-09-20T11:03:41Z

app/scripts/services/quota.js

+      var msgPrefix = "Your project is " + (hardValue < usedValue ? 'over' : 'at') + " quota. ";
+      var msg;
+      if (_.includes(COMPUTE_RESOURCE_QUOTAS, quotaKey)) {
+        msg = msgPrefix + "It is using " + percent((usedValue/hardValue), 0) + " of " + usageWithUnits(hard, quotaKey) + " " + humanizeQuotaResource(quotaKey, true) + ".";


Let's not pass true to humanizeResourceQuota here and just below. We typically don't capitalize resource kinds in the console in sentences, only headings.

spadgett · 2017-09-20T11:16:29Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+        _.each(drawer.notificationGroups, function(group) {
+          _.remove(group.notifications, function(grpNotification) {
+            return grpNotification.uid === notification.uid && grpNotification.namespace === notification.namespace;
+          });


Nit: this could be

_.remove(group.notifications, { uid: notification.uid, namespace: notification.namespace });

spadgett · 2017-09-20T11:19:20Z

I noticed that the dropdown can get clipped if there's only one notification:

dtaylor113 · 2017-09-20T13:58:53Z

I noticed that the dropdown can get clipped if there's only one notification:

Hi, yes, filed a-pf issue: patternfly/angular-patternfly#614

dtaylor113 · 2017-09-20T14:07:27Z

Hi @spadgett, addressed your latest comments. Still have a question about escaping the custom message though.

spadgett · 2017-09-20T14:16:22Z

Hi, yes, filed a-pf issue: patternfly/angular-patternfly#614

Thanks. I'm not 100% sure it's a a-pf bug since we're adding the dropdown.

cc @rhamilto

dtaylor113 · 2017-09-20T14:24:43Z

Thanks. I'm not 100% sure it's a a-pf bug since we're adding the dropdown.

I see the clipping in our a-pf example with the notification drawer. I spend a little time trying overflow and z-index properties but couldn't get it to work the way I wanted.

spadgett

Nice work @dtaylor113. Really happy to see this going in.

One comment post rebase.

spadgett · 2017-09-20T14:38:13Z

app/views/directives/notifications/notification-body.html

@@ -5,6 +5,8 @@
  <a
    class="pull-right"
    href=""
+    ng-if="!notification.actions.length"
+    tabindex="0"


Remove tabindex. It's not needed now that we have an href. Looks like this was accidentally added back on rebase

Ok, removed tabindex. -thanks

spadgett · 2017-09-20T14:57:35Z

[merge]

dtaylor113 · 2017-09-20T15:10:38Z

Filed these issues to cover remaining work:

Quota Notifications: Resetting/Restoring "Don't Show Me Again" notifications
Quota Notifications: Add kabob menu to Toasts
Quota Notifications: Toast when user takes action which reaches quota

spadgett · 2017-09-20T15:33:51Z

WebDriver flake

[merge]

openshift-bot · 2017-09-20T15:39:33Z

Evaluated for origin web console merge up to b4407c9

openshift-bot · 2017-09-20T15:39:45Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/234/) (Base Commit: 1ceb0ff) (PR Branch Commit: b4407c9)

benjaminapetersen · 2017-10-11T17:28:45Z

app/scripts/constants.js

@@ -555,5 +555,10 @@ angular.extend(window.OPENSHIFT_CONSTANTS, {
    //   href: 'http://example.com/',
    //   tooltip: 'Open Dashboard'
    // }
-  ]
+  ],
+  QUOTA_NOTIFICATION_MESSAGE: {


Did we intend to leave this in as an empty object?

I believe so. Similar to the other two empty objects right before this one.

Yeah, they're for custom messages. We generate a default message in code.

dtaylor113 force-pushed the quotas4 branch from 72741ab to 81a3db1 Compare September 19, 2017 14:21

dtaylor113 commented Sep 19, 2017

View reviewed changes

spadgett requested changes Sep 19, 2017

View reviewed changes

dtaylor113 force-pushed the quotas4 branch from 81a3db1 to 2985078 Compare September 19, 2017 18:22

dtaylor113 force-pushed the quotas4 branch from 2985078 to ab47734 Compare September 19, 2017 18:36

spadgett requested changes Sep 19, 2017

View reviewed changes

spadgett reviewed Sep 19, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2017

dtaylor113 force-pushed the quotas4 branch from ab47734 to 4c0fa55 Compare September 19, 2017 20:27

spadgett reviewed Sep 20, 2017

View reviewed changes

spadgett mentioned this pull request Sep 20, 2017

"Quota limit has been reached" gets easily misunderstood / ignored openshift/origin#13976

Closed

dtaylor113 force-pushed the quotas4 branch from 4c0fa55 to b950ff6 Compare September 20, 2017 14:06

dtaylor113 changed the title ~~[WIP] Quota Notifications~~ Quota Notifications Sep 20, 2017

dtaylor113 force-pushed the quotas4 branch from b950ff6 to c1fd134 Compare September 20, 2017 14:30

spadgett reviewed Sep 20, 2017

View reviewed changes

Quota Notifications

b4407c9

dtaylor113 force-pushed the quotas4 branch from c1fd134 to b4407c9 Compare September 20, 2017 14:47

spadgett removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2017

spadgett approved these changes Sep 20, 2017

View reviewed changes

openshift-bot merged commit e9c7620 into openshift:master Sep 20, 2017

This was referenced Sep 20, 2017

Need to document Custom Notification openshift/openshift-docs#5307

Closed

Need to document how to configure Custom Quota Notification messages openshift/openshift-docs#5308

Closed

benjaminapetersen reviewed Oct 11, 2017

View reviewed changes

dtaylor113 deleted the quotas4 branch October 19, 2017 18:30

damemi mentioned this pull request Jul 12, 2018

Add free and paid online-extensions.js files for custom quota messages openshift/online-console-extensions#40

Merged

spadgett mentioned this pull request Aug 18, 2022

Added resource quota warning message in topology and add page openshift/console#11962

Merged

4 tasks

	.filter('percent', function() {
	// Takes a number like 0.33 and returns "33%". `precision` is the optional
	// number of digits to appear after the decimal point.
	return function(value, precision) {
	if (value === null \|\| value === undefined) {
	return value;
	}
	return _.round(Number(value) * 100, precision) + "%";
	};
	})

Quota Notifications #2116

Quota Notifications #2116

Conversation

dtaylor113 commented Sep 19, 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

dtaylor113 commented Sep 19, 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

dtaylor113 commented Sep 19, 2017

spadgett left a comment

Choose a reason for hiding this comment

spadgett Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtaylor113 Sep 19, 2017 • edited Loading

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

dtaylor113 commented Sep 19, 2017

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

Choose a reason for hiding this comment

dtaylor113 Sep 20, 2017 • edited Loading

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 Sep 20, 2017

dtaylor113 commented Sep 20, 2017

dtaylor113 commented Sep 20, 2017

spadgett commented Sep 20, 2017

dtaylor113 commented Sep 20, 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

spadgett commented Sep 20, 2017

dtaylor113 commented Sep 20, 2017

spadgett commented Sep 20, 2017

openshift-bot commented Sep 20, 2017

openshift-bot commented Sep 20, 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 Sep 19, 2017 •

edited

Loading

spadgett Sep 19, 2017 •

edited

Loading

dtaylor113 Sep 19, 2017 •

edited

Loading

dtaylor113 Sep 20, 2017 •

edited

Loading

dtaylor113 commented Sep 20, 2017 •

edited

Loading

openshift-bot commented Sep 20, 2017 •

edited

Loading