Refactors dashboard state to a component #1373

AllenBW · 2018-01-25T22:43:08Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540733

In support of the longer term fix brought on by #1372

So there's a few bzs this fixes (for real this time):

look, its working 🍰

Goal of this is to be identical to original, cept no more 403 kick ya outta the app state change errors

TODO 👍 : (In this)

add the basic component, put all the existing logic in the right places
build out the dashboard.component.service
refactor dashboard.component.spec
ensure dashboard.component is working as intended

miq-bot · 2018-01-26T15:35:17Z

This pull request is not mergeable. Please rebase and repush.

AllenBW · 2018-01-28T18:38:20Z

Thinking its probably a good idea to get @serenamarie125 to check this out also... specifically the loading of the state before the widgets are present, maybe each column should have a loading spinner?

Or maybe not?

chalettu

Never mind original comment

serenamarie125 · 2018-01-30T17:36:01Z

@AllenBW thanks for bringing this up ! From a PatternFly perspective, the cards should be displayed with their loading state, then once the data is available ... 🐰 🎩

The base card describes this state here: http://www.patternfly.org/pattern-library/cards/base-card/#design

serenamarie125 · 2018-01-30T17:48:58Z

FYI - After chatting with @AllenBW as well as some of the visual designers, we will look into an alternate solution from a PF perspective - possibly a way to tie cards together, so that if data isn't available, you can only show a single spinner. This won't be trivial and a design should be done up front, implemented in PF and then be brought back to product. So for now, let's stick with the PF solution. Sound good ?

AllenBW · 2018-01-30T17:54:55Z

@serenamarie125 so present state, these aren't actually pf cards... you ok with the top two "header" cards being swapped for the design of the pfInfoStatusCard? http://www.patternfly.org/angular-patternfly/#/api/patternfly.card.component:pfInfoStatusCard

ok swapping is a bad idea, gonna add a real quick loading state, we'll revisit this in the near future 😁

AllenBW · 2018-01-30T18:52:05Z

@serenamarie125 updated the first comment to have a new gif, showing loading state! also... if the user doesn't have permissions to view the card, its removed when loading fails

serenamarie125

This is a thing of beauty! LGTM @AllenBW

serenamarie125 · 2018-01-30T20:19:35Z

@miq-bot add_label ux/approved

Removes nasty polling, still isn't a solid fix, but better

chalettu

Small little change for readability but this looks like a great change for how dashboards work. Great job!

chalettu · 2018-02-02T15:16:34Z

client/app/core/navigation.service.js

@@ -123,4 +130,10 @@ export function NavigationFactory (RBAC, Polling, POLLING_INTERVAL, $ngRedux, Co
      })
    })
  }
+
+  function setPermissions () {
+    menuItems[1].permissions = RBAC.has(RBAC.FEATURES.SERVICES.VIEW)


Can we create constants for these array positions. I know its simple but it will make the code a little easier to read.

The assignment is self informing, I don't so much agree with creating variables that server no other purpose than to increase verbosity .

dclarizio · 2018-02-02T20:48:27Z

@himdel your attention required on this blocker PR . . . thx, Dan

himdel · 2018-02-05T11:42:44Z

So... before reviewing this any further... do we need to gaprindashvili/yes this?

I mean, it's great to see code rewritten to components.
It's just not so great as a blockers-only bugfix...

himdel · 2018-02-05T12:45:01Z

client/gettext/po/manageiq-ui-service.pot

@@ -4,7 +4,7 @@ msgstr ""
 "Content-Transfer-Encoding: 8bit\n"
 "Project-Id-Version: \n"

-#: ./client/app/states/dashboard/dashboard.html:88
+#: ./client/app/components/dashboard/dashboard.html:88


recreating pot files should not be a part of the PR

@himdel will remove, but why not? especially when file refactoring takes place?

AllenBW · 2018-02-05T13:12:47Z

@himdel agreed its a large change to be a bugfix, but removing the "application gateway" (the three asset resolves that challenged a user enters entering dashboard state) state resolves is the best way to fix the bz..

When any of those fails, the app router throws us into a $stateChangeError loop. Unfortunately throwing a event.preventDefault() will not address this infinite loop. These calls have to be allowed to fail, means we need to get em outta the resolve, means dashboard now has to be a component.

himdel · 2018-02-05T14:48:01Z

@AllenBW So.. if we just backported a change that prevents the resolve from erroring..?

Something like

 function resolveServicesWithDefinedServiceIds (CollectionsApi, RBAC) {
   if (RBAC.has('service_view') && RBAC.has(RBAC.FEATURES.SERVICES.VIEW)) {
     const options = {
       expand: 'resources',
       filter: ['service_id=nil'],
       attributes: ['chargeback_report']
     }
 
     return CollectionsApi.query('services', options)
+      .catch(function() {
+         return null;
+      });
   }
 
   return true
 }

?

miq-bot · 2018-02-05T15:18:11Z

Checked commits AllenBW/manageiq-ui-service@e8962bb~...6e55968 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

himdel

Verified user switching now works correctly, regardless of the initial login role.

LGTM, thanks @AllenBW :)

himdel · 2018-02-05T17:47:07Z

@AllenBW One thing I do notice..

log in with EvmGroup-desktop
theres no count shown for My Services in the menu (as that role can't access services)

but..

log in with any other role
switch to EvmGroup-desktop
the count is there (but clicking the menu item still gives me "There was an error loading the services. Use of the read action is forbidden")

Not sure if this is just about removing the count (but that doesn't sound blocker-worthy), or if the menu item should not be there (but that would be a separate bug entirely).

simaishi · 2018-02-05T20:00:56Z

So.. if we just backported a change that prevents the resolve from erroring..?

@AllenBW @himdel since I don't see a reply to that question... can either one of you confirm that we will be backporting the entire PR to Gaprindashvili branch?

AllenBW · 2018-02-05T20:48:17Z

@simaishi yes, whole pr, apologies for delayed response 😭

simaishi · 2018-02-05T23:03:11Z

@AllenBW @himdel @dclarizio

This is built on top of #1375, which is not in Gaprindashvili yet (not blocker), and can't be backported as is. I think we have 2 options here... Either 1) we create a separate PR for Gaprindashvili without #1375 (not sure how much work that is...), or 2) we take #1375 now and backport this PR as well.

Two PRs touch many files/lines and I'm not sure how 'safe' it is to get either/both PRs to gaprindashvili branch now. Looking for a guidance from someone familiar with both changes...

cc @chrispy1

AllenBW · 2018-02-05T23:26:41Z

To answer the how much work question (@simaishi ), wouldn't say more than a few hours of effort to copy this into a Gaprindashvili specific branch, the only overlap is a single file, and even that is not more than 5 or so lines

himdel · 2018-02-06T13:53:59Z

@AllenBW Can you take care of creating a gaprindashvili version of this?

ManageIQ#1373 for Gaprindashvili

simaishi · 2018-02-06T16:05:22Z

Backported to Gaprindashvili via #1378

AllenBW requested a review from himdel as a code owner January 25, 2018 22:43

miq-bot added the wip label Jan 25, 2018

AllenBW added the enhancement label Jan 25, 2018

AllenBW force-pushed the enhancement/master/#1533222-componentize-dashboard branch from 46fc4b7 to 7c0e33a Compare January 26, 2018 14:06

miq-bot added the unmergeable label Jan 26, 2018

AllenBW force-pushed the enhancement/master/#1533222-componentize-dashboard branch from 7c0e33a to f6f7953 Compare January 26, 2018 15:37

miq-bot removed the unmergeable label Jan 26, 2018

AllenBW force-pushed the enhancement/master/#1533222-componentize-dashboard branch 3 times, most recently from 89e71b9 to b68fddf Compare January 26, 2018 22:11

AllenBW assigned himdel Jan 27, 2018

AllenBW removed the wip label Jan 28, 2018

AllenBW changed the title ~~[WIP] Refactors dashboard state to a component~~ Refactors dashboard state to a component Jan 28, 2018

AllenBW added bug blocker gaprindashvili/yes labels Jan 28, 2018

AllenBW requested a review from chalettu January 28, 2018 12:59

AllenBW requested a review from serenamarie125 January 28, 2018 18:38

AllenBW added the ux/review label Jan 28, 2018

AllenBW added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 29, 2018

chalettu reviewed Jan 29, 2018

View reviewed changes

AllenBW force-pushed the enhancement/master/#1533222-componentize-dashboard branch from 4684a2a to 6d3312e Compare January 30, 2018 18:57

serenamarie125 approved these changes Jan 30, 2018

View reviewed changes

AllenBW force-pushed the enhancement/master/#1533222-componentize-dashboard branch from 8c6a1a9 to f450f6e Compare February 2, 2018 13:51

Rebase cleanup

e72fe1b

AllenBW force-pushed the enhancement/master/#1533222-componentize-dashboard branch from f450f6e to e72fe1b Compare February 2, 2018 13:52

miq-bot removed the unmergeable label Feb 2, 2018

Refactor nav ctrl to leverage in place docheck

7c05328

Removes nasty polling, still isn't a solid fix, but better

chalettu suggested changes Feb 2, 2018

View reviewed changes

himdel reviewed Feb 5, 2018

View reviewed changes

Revert .pot

6e55968

himdel approved these changes Feb 5, 2018

View reviewed changes

himdel merged commit 924fee0 into ManageIQ:master Feb 5, 2018

AllenBW deleted the enhancement/master/#1533222-componentize-dashboard branch February 5, 2018 17:59

simaishi added the gaprindashvili/conflict label Feb 5, 2018

AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Feb 6, 2018

Componetizes the dashboard to fix group switching and lnav

a8f26e9

ManageIQ#1373 for Gaprindashvili

AllenBW mentioned this pull request Feb 6, 2018

[GAPRINDASHVILI] Componetizes the dashboard to fix group switching and lnav #1378

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/conflict labels Feb 6, 2018

AllenBW mentioned this pull request Mar 14, 2018

Change how Ajax requests are run on Dashboard State #1151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors dashboard state to a component #1373

Refactors dashboard state to a component #1373

AllenBW commented Jan 25, 2018 •

edited

Loading

miq-bot commented Jan 26, 2018

AllenBW commented Jan 28, 2018 •

edited

Loading

chalettu left a comment •

edited

Loading

serenamarie125 commented Jan 30, 2018 •

edited

Loading

serenamarie125 commented Jan 30, 2018

AllenBW commented Jan 30, 2018 •

edited

Loading

AllenBW commented Jan 30, 2018

serenamarie125 left a comment

serenamarie125 commented Jan 30, 2018

chalettu left a comment

chalettu Feb 2, 2018

AllenBW Feb 2, 2018

dclarizio commented Feb 2, 2018

himdel commented Feb 5, 2018

himdel Feb 5, 2018

AllenBW Feb 5, 2018

AllenBW commented Feb 5, 2018

himdel commented Feb 5, 2018

miq-bot commented Feb 5, 2018

himdel left a comment

himdel commented Feb 5, 2018 •

edited

Loading

simaishi commented Feb 5, 2018

AllenBW commented Feb 5, 2018 •

edited

Loading

simaishi commented Feb 5, 2018

AllenBW commented Feb 5, 2018

himdel commented Feb 6, 2018

simaishi commented Feb 6, 2018

Refactors dashboard state to a component #1373

Refactors dashboard state to a component #1373

Conversation

AllenBW commented Jan 25, 2018 • edited Loading

look, its working 🍰

miq-bot commented Jan 26, 2018

AllenBW commented Jan 28, 2018 • edited Loading

chalettu left a comment • edited Loading

Choose a reason for hiding this comment

serenamarie125 commented Jan 30, 2018 • edited Loading

serenamarie125 commented Jan 30, 2018

AllenBW commented Jan 30, 2018 • edited Loading

AllenBW commented Jan 30, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

serenamarie125 commented Jan 30, 2018

chalettu left a comment

Choose a reason for hiding this comment

chalettu Feb 2, 2018

Choose a reason for hiding this comment

AllenBW Feb 2, 2018

Choose a reason for hiding this comment

dclarizio commented Feb 2, 2018

himdel commented Feb 5, 2018

himdel Feb 5, 2018

Choose a reason for hiding this comment

AllenBW Feb 5, 2018

Choose a reason for hiding this comment

AllenBW commented Feb 5, 2018

himdel commented Feb 5, 2018

miq-bot commented Feb 5, 2018

himdel left a comment

Choose a reason for hiding this comment

himdel commented Feb 5, 2018 • edited Loading

simaishi commented Feb 5, 2018

AllenBW commented Feb 5, 2018 • edited Loading

simaishi commented Feb 5, 2018

AllenBW commented Feb 5, 2018

himdel commented Feb 6, 2018

simaishi commented Feb 6, 2018

AllenBW commented Jan 25, 2018 •

edited

Loading

AllenBW commented Jan 28, 2018 •

edited

Loading

chalettu left a comment •

edited

Loading

serenamarie125 commented Jan 30, 2018 •

edited

Loading

AllenBW commented Jan 30, 2018 •

edited

Loading

himdel commented Feb 5, 2018 •

edited

Loading

AllenBW commented Feb 5, 2018 •

edited

Loading