Add track by to notification drawer #1988

benjaminapetersen · 2017-08-29T21:17:58Z

re issue #1981

Working on testing with a high volume of events to ensure it performs as expected.

jeff-phillips-18 · 2017-08-29T21:22:57Z

Working on getting patternfly/angular-patternfly#602 available to you in console.

jeff-phillips-18 · 2017-08-29T21:32:21Z

Did you have issue upgrading the angular patternfly version? I got bower install issues:

`Please note that,
origin-web-catalog#0.0.41, origin-web-console depends on angular-bootstrap#0.14.3 which resolved to angular-bootstrap#0.14.3
angular-patternfly#4.7.1, angular-patternfly#4.5.6 depends on angular-bootstrap#2.2.x which resolved to angular-bootstrap#2.2.0
Resort to using angular-bootstrap#0.14.3 which resolved to angular-bootstrap#0.14.3
Code incompatibilities may occur.

bower ECONFLICT Unable to find suitable version for angular-patternfly
`

I am trying to update origin-web-catalog first.

spadgett · 2017-08-29T22:21:16Z

@benjaminapetersen You don't necessarily need a high volume. You just need one event that updates ("seen 23 times in the last 2 hours") like a crash looping pod.

spadgett · 2017-08-29T22:23:27Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

          if(EventsService.isImportantEvent(event) && !EventsService.isCleared(event)) {
            ensureProjectGroupExists(filtered, event.metadata.namespace);
            filtered[event.metadata.namespace].notifications.push({
              unread:  !EventsService.isRead(event),
+              uid: eventId,


I'd prefer event.metadata.uid here so it doesn't matter how the map is keyed. uid seems safest since it's globally unique.

Agree, updating this.

benjaminapetersen · 2017-08-30T01:31:38Z

@jeff-phillips-18 yup, i did. Prob need to update catalog & all too 😕
I wonder if, for common & catalog, we could set the bower dependencies with greater than:

"angular-patternfly": ">4.3.0",  # at least this, but later is fine!
"angular-patternfly": "~4.3.0",  # current form, tilde limits us to patches only...

This would still allow the origin-web-console to be the single source of truth & keep our builds consistent, but might get us out of the need to update the 2 secondary repos with change?

spadgett · 2017-08-31T14:18:37Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -154,11 +154,12 @@
      var formatAndFilterEvents = function(eventMap) {
        var filtered = {};
        ensureProjectGroupExists(filtered, $routeParams.project);
-        _.each(eventMap, function(event) {
+        _.each(eventMap, function(event, eventUID) {


I'd still rather just use event.metadata.uid below instead of relying on the map being keyed by UID. Then if it ever changes later in eventWatchCallback, this function won't be affected.

I don't think the changes to the notification drawer allow for a nested field. @jeff-phillips-18

Code here looks like it has to be top level on the object:

ng-repeat="notification in notificationGroup.notifications track by $ctrl.notificationTrackField ? notification[$ctrl.notificationTrackField] || $index : $index"

If I'm following what you are saying.

benjaminapetersen · 2017-08-31T18:14:43Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

@@ -159,6 +159,7 @@
            ensureProjectGroupExists(filtered, event.metadata.namespace);
            filtered[event.metadata.namespace].notifications.push({
              unread:  !EventsService.isRead(event),
+              uid: event.metadata.uid,


@spadgett fixed

spadgett · 2017-08-31T19:08:34Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

-        eventsByNameData = eventData.by('metadata.name');
-        eventsMap = formatAndFilterEvents(eventsByNameData);
+        eventsByUID = eventData.by('metadata.uid');
+        eventsMap = formatAndFilterEvents(eventsByUID);


It's better to leave this as eventsByName unless we need it mapped by UID because data.by('metadata.name') is a no-op. data.by('metadata.uid') forces the map to be re-keyed.

Being a bit nit picky here because I'm afraid this might get called frequently.

I'd make it a local var to this function if it's not used outside the function.

I can swap it back here (though I think I'm going to turn it right back to uid in my next PR adding the other kind of notifications, but will see if I can leave it .name 😄 )

OK that's fine then as long as it's needed

spadgett · 2017-09-01T13:34:46Z

[test]

spadgett · 2017-09-01T14:06:56Z

@benjaminapetersen You also need to bump origin-web-catalog that @jeff-phillips-18 released to avoid the bower conflict

benjaminapetersen · 2017-09-01T14:31:25Z

bumped

[test] again

spadgett · 2017-09-01T14:36:47Z

[merge][severity: bug]

openshift-bot · 2017-09-01T14:56:29Z

Origin Web Console Test Results: Waiting: Determining build queue position

spadgett · 2017-09-01T14:59:40Z

[merge][severity: bug]

spadgett · 2017-09-01T16:13:13Z

@benjaminapetersen We have a conflict in bower.json now :(

benjaminapetersen · 2017-09-01T17:53:40Z

Heh, resolving....

benjaminapetersen · 2017-09-01T17:59:09Z

[test]

benjaminapetersen · 2017-09-01T18:11:05Z

Welp, lets see...

benjaminapetersen · 2017-09-01T18:17:59Z

Already fixed this:

        "CONFLICT (content): Merge conflict in dist/scripts/scripts.js", 
        "Auto-merging bower.json", 
        "CONFLICT (content): Merge conflict in bower.json",

[test] again?

benjaminapetersen · 2017-09-01T18:55:03Z

😐 😒

spadgett · 2017-09-01T20:10:29Z

@benjaminapetersen Make sure you bower install again if you haven't. A couple bower updates went in today.

benjaminapetersen · 2017-09-01T20:15:03Z

@spadgett yup, I'm rebased on master, hack/clean/install multiple times. Something is a little wonky, trying to figure out what.

spadgett · 2017-09-01T20:58:57Z

@benjaminapetersen Make sure you've rebased on latest master, too, even if there aren't merge conflicts. Sometimes the uglified output changes.

benjaminapetersen · 2017-09-05T18:37:48Z

Pretty sure this is a flake:

FAIL
   Error: cannot generate registry resources
   Caused By:
     Error: Docker exec error
     Details:
       Container: origin
       Command: [oc adm registry --dry-run --output=json --images=openshift/origin-${component}:latest --mount-host=/var/lib/origin/openshift.local.pv/registry]
       Result Code: 137

++ export status=FAILURE
++ status=FAILURE
+ set +o xtrace
########## FINISHED STAGE: FAILURE: START OPENSHIFT SERVER [00h 04m 16s] ##########
Build step 'Execute shell' marked build as failure
[PostBuildScript] - Execution post build scripts.

[test]

spadgett · 2017-09-05T18:38:59Z

Tests will probably fail until #2003 merges

openshift-bot · 2017-09-05T18:40:21Z

Evaluated for origin web console test up to 362717c

openshift-bot · 2017-09-05T19:16:56Z

Origin Web Console Test Results: FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/128/) (Base Commit: 5709d8a) (PR Branch Commit: 362717c)

spadgett · 2017-09-06T01:28:29Z

[merge][severity: blocker]

openshift-bot · 2017-09-06T01:32:21Z

Evaluated for origin web console merge up to 362717c

openshift-bot · 2017-09-06T03:56:16Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/112/) (Base Commit: 6ff495e) (PR Branch Commit: 362717c) (Extended Tests: blocker, bug)

spadgett reviewed Aug 29, 2017

View reviewed changes

benjaminapetersen force-pushed the bpeterse/issue/1981/notification-drawer-flicker branch from ce48f65 to 8bfd50b Compare August 30, 2017 01:40

benjaminapetersen mentioned this pull request Aug 30, 2017

Update angular-patternfly dependency to "at least" syntax openshift/origin-web-catalog#408

Merged

spadgett mentioned this pull request Aug 31, 2017

Bump origin-web-common to 0.0.51 #1992

Merged

spadgett reviewed Aug 31, 2017

View reviewed changes

benjaminapetersen force-pushed the bpeterse/issue/1981/notification-drawer-flicker branch from 8bfd50b to 94d3de8 Compare August 31, 2017 18:13

benjaminapetersen commented Aug 31, 2017

View reviewed changes

spadgett reviewed Aug 31, 2017

View reviewed changes

benjaminapetersen force-pushed the bpeterse/issue/1981/notification-drawer-flicker branch from 94d3de8 to a7f96ff Compare September 1, 2017 13:31

spadgett approved these changes Sep 1, 2017

View reviewed changes

benjaminapetersen force-pushed the bpeterse/issue/1981/notification-drawer-flicker branch from a7f96ff to 8dc5fca Compare September 1, 2017 14:31

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

benjaminapetersen force-pushed the bpeterse/issue/1981/notification-drawer-flicker branch from 8dc5fca to 9b6611a Compare September 1, 2017 17:57

benjaminapetersen mentioned this pull request Sep 1, 2017

Fix the hack/verify-dist.sh message #1999

Merged

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

benjaminapetersen force-pushed the bpeterse/issue/1981/notification-drawer-flicker branch from 9b6611a to 362717c Compare September 5, 2017 14:41

Add track by to notification drawer

362717c

openshift-bot merged commit 4d49437 into openshift:master Sep 6, 2017

benjaminapetersen deleted the bpeterse/issue/1981/notification-drawer-flicker branch September 6, 2017 14:02

Add track by to notification drawer #1988

Add track by to notification drawer #1988

Conversation

benjaminapetersen commented Aug 29, 2017

jeff-phillips-18 commented Aug 29, 2017

jeff-phillips-18 commented Aug 29, 2017

spadgett commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Aug 30, 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

spadgett commented Sep 1, 2017

spadgett commented Sep 1, 2017

benjaminapetersen commented Sep 1, 2017

spadgett commented Sep 1, 2017

openshift-bot commented Sep 1, 2017 • edited Loading

spadgett commented Sep 1, 2017

spadgett commented Sep 1, 2017

benjaminapetersen commented Sep 1, 2017

benjaminapetersen commented Sep 1, 2017

benjaminapetersen commented Sep 1, 2017

benjaminapetersen commented Sep 1, 2017

benjaminapetersen commented Sep 1, 2017

spadgett commented Sep 1, 2017

benjaminapetersen commented Sep 1, 2017

spadgett commented Sep 1, 2017

benjaminapetersen commented Sep 5, 2017

spadgett commented Sep 5, 2017

openshift-bot commented Sep 5, 2017

openshift-bot commented Sep 5, 2017

spadgett commented Sep 6, 2017

openshift-bot commented Sep 6, 2017

openshift-bot commented Sep 6, 2017

openshift-bot commented Sep 1, 2017 •

edited

Loading