[IMPAC-757] Added radio mode for organization

[MAhsenArif]

Radio mode can be set on Widgets by adding parameter organization-mode="'single'" to setting-organizations directive.

[xaun]

Thanks for your PR @MAhsenArif, some changes on this one, mainly just to refactor the big block in the initializer. Cheers

[xaun]

@MAhsenArif is it apart of the specification for this "single" mode to be not used anywhere in the project? Just as it's being set here, and when I search the project there are no widgets that bind an organizationMode property to this directive.

[MAhsenArif]

@xaun Currently I've have added the ability to use single mode but yes it's not being used anywhere. As per discussion with @cesar-tonnoir , this is something the developers will activate when coding a specific widget.

I've also added a message in PR description on how it can be used for a widget.

[xaun]

Is it worth adding another line here when the below functions could access $scope.organizationMode directly?

[MAhsenArif]

Added it as a bit of readability. $scope.organizationMode was too long to be used everywhere :)

[xaun]

I think Lodash _.each outperforms angular.forEach, might as well use it as it's the standard for this stuff in impac-angular.

But for this circumstance, there is an even better Lodash method _.mapValues.

This could simply be _.mapValues(w.selectedOrganizations, -> false)

[xaun]

This block could do with some refactoring, simplifying and commenting. The code is not very readable, and will cause future confusion for developers.

After looking into for a bit, Im not even sure it's necessary to be iterating over all the $scope.dashboardOrganizations and cramming all the logic into all the if's & else's, which Im not even sure we need all of.

I have tried to capture all the cases you have coded for in comments:

# single mode 
# - has multiple data sources previously selected 
# => deselect all & select current organization
# - none or 1 data source previously selected
# => apply the previously first widget data source organization, or select the first one

Is this correct?

We shouldn't have to "deselect" (e.g set all the key values to false), as this is inside an initialize method, no "checkboxes" are going to be ticked, right? This saves us having to iterate and set everything to false, and having to use a counter.

The thing with an ng-model binding, is that if it is undefined or null, it's falsey. We should only need to set it when it's been toggled for the first time.

I may not have capture all the cases, but this is a quick proposition of a refactor:

if w.metadata? && w.metadata.organization_ids?
  if singleOrgMode()
    currentOrganization = ImpacMainSvc.config.currentOrganization
    unless _.isEmpty(currentOrganization)
      w.selectedOrganizations[currentOrganization.uid] = true
    else if (dhbOrg = $scope.dashboardOrganizations[0])?
      w.selectedOrganizations[dhbOrg.uid] = true

Anyway I may be completely missing something, but if you could just define & code the logic in a clearer manner that would be awesome! Thanks :)

[xaun]

Thanks @MAhsenArif, LGTM..

Just a couple of suggestions & a question before merge. Let me know what you think

[xaun]

why not just if widgetOrgIds.length > 1? What's the _.map for? If w.metadata.organization_ids is defined, it should always be an Array.

[xaun]

Might be nice to flatten this a bit by going:

return unless w.metadata? && w.metadata.organization_ids?

Also I've noticed at the bottom of this callback, setting.isInitialized = true is nested within the if block, so if there are no w.metadata.organization_ids, it will never set itself as isInitialized.

Not sure if this is intended or not. But I think maybe we could move this into a .finally() callback?

ImpacDashboardSvc.load().then(
    (config)->
         ...
).finally(->
   settings.isInitialized = true
)

What do you think?

[MAhsenArif]

@xaun Agree with setting isInitialized in finally block. Though I'm not yet sure what it's purpose is. I searched for it but it is not used in any views.

[xaun]

@MAhsenArif good point, it seems only the widgets-settings/accounts-list setting uses it in its settings.toMetadata method.

I think this was an old framework specific convention of sorts.. @cesar-tonnoir can you confirm if there is any reason to keep this property? It looks like our newer widgets-settings components are not implementing it? If not, we should remove these from the old widgets.

[xaun]

Maybe use else if like you have in the $scope.toggleSelectOrganization method.