Skip to content

[MNOE-1280] Cleared instances cache to refresh apps & products#541

Open
RidaAhmad wants to merge 3 commits into
maestrano:2.0from
RidaAhmad:mnoe-1280-refresh-app-and-product-instances
Open

[MNOE-1280] Cleared instances cache to refresh apps & products#541
RidaAhmad wants to merge 3 commits into
maestrano:2.0from
RidaAhmad:mnoe-1280-refresh-app-and-product-instances

Conversation

@RidaAhmad

@RidaAhmad RidaAhmad commented Aug 28, 2018

Copy link
Copy Markdown
  • Emptied out apps & products instances' cache upon subscription's validation so the instances are fetched again and hence the updated apps are displayed on dock & app management page.
  • Removed redundant reloading of app instances

$state.go("home.subscriptions", {subType: 'cart'})
else
clearInstancesCache()
# Reload dock apps

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MAhsenArif Please if you can confirm if we're good to remove this reload dock apps block of code as it no longer seems to be required now, will appreciate your input on this so you can confirm if it ain’t needed for cart or any other related functionalities.
Thank you,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RidaAhmad We still need this for the impac dashboard(if it is enabled) as this code is responsible for reloading the dock apps.
It isnt, however related to the cart subscriptions reloading as that is a separate condition branch(the if part of this code).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thank you!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RidaAhmad On a deeper look, it seems that since we have a watcher added in the app docks component, it does do the appInstance cache refresh on it's own. This watcher will always get triggered because the app dock component is completely removed from DOM and re-added whenever we switch to and fro from Impac Dasboard.

The same behaviour is happening for the apps dashboard(tile view)

So it should be safe to remove the dock reload from here. and just keep the clearInstancesCache() method.

FYI @adamaziz15 @ouranos

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MAhsenArif Yeah, seems to work fine without having to reload dock apps from here.
We can remove it for now and revert in case of any issue later.
Thank you for your input.

@RidaAhmad RidaAhmad force-pushed the mnoe-1280-refresh-app-and-product-instances branch from 303bfbc to 3ee96a3 Compare August 29, 2018 06:27
@@ -1,5 +1,5 @@
angular.module 'mnoEnterpriseAngular'
.controller('ProvisioningConfirmCtrl', ($scope, $state, $stateParams, $log, MnoeOrganizations, MnoeProvisioning, MnoeAppInstances, MnoeConfig, ProvisioningHelper, schemaForm, toastr) ->
.controller('ProvisioningConfirmCtrl', ($scope, $state, $stateParams, $log, MnoeOrganizations, MnoeProvisioning, MnoeAppInstances, MnoeConfig, ProvisioningHelper, schemaForm, toastr, MnoeProductInstances) ->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line size increased. Might need to adjust indentation of the file.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback incorporated, thanks.

@RidaAhmad RidaAhmad changed the title [MNOE-1280] [WIP] Cleared instances cache to refresh apps & products [MNOE-1280] Cleared instances cache to refresh apps & products Aug 29, 2018
@ouranos ouranos added this to the Opal SoW2 - Backlog milestone Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants