[APPINT-1044] Update frontend and components to display info about add-ons #352

manu-d · 2017-10-04T15:39:55Z

Two main modals:

Added app-connect-modal-cloud
Updated app-settings-modal

manu-d · 2017-10-04T15:41:12Z

For the app-settings-modal, you can just look at the coffee controller for the moment. The html and css need to be completely rewritten.

alexnoox

Hi @manu-d,

Please see my comments. I will do another review later to see if we can avoid code duplication using components.

TODO:

I18n all the texts

alexnoox · 2017-10-10T05:46:47Z

src/app/components/dashboard-apps-dock/dashboard-apps-dock.coffee

-    instance.stack == 'connector' &&
-    !instance.oauth_keys_valid
+    (instance.app_nid != 'office-365' && instance.stack == 'connector' && !instance.oauth_keys_valid) ||
+    (instance.add_on && instance.organization && !instance.organization.has_account_linked)


You could reuse $scope.helper.isCreateAccountShown

alexnoox · 2017-10-10T06:05:46Z

src/app/components/dashboard-apps-dock/dashboard-apps-dock.html

@@ -9,10 +9,13 @@
          <div class="dock-arrow"><i class="fa fa-chevron-up"></i></div>
          <div class="inner-dock-menu">
            <!-- EO Connectors Buttons -->
+            <div class="menu-item ng-hide" ng-show="helper.isCreateAccountShown(app)" uib-tooltip="{{ 'mno_enterprise.templates.components.app_install_btn.create_account_tooltip' | translate }}">


Remove the .ng-hide class

It seems there .ng-hide class everywhere in this file. Please remove them.

alexnoox · 2017-10-10T06:07:26Z

src/app/components/dashboard-apps-dock/dashboard-apps-dock.less

@@ -129,6 +129,8 @@ ul.dock {
      .menu-item {
        cursor: pointer;



Remove empty line

alexnoox · 2017-10-10T06:24:14Z

src/app/components/mnoe-api/app-instances.svc.coffee

+      MnoeApiSvc.one('organizations', MnoeOrganizations.selectedId).one('/app_instances', instance.id).one('/setup_form').get()
+
+    @submitForm = (instance, model) ->
+      body = {}


Please use data instead of body

alexnoox · 2017-10-10T06:27:26Z

src/app/components/mnoe-api/app-instances.svc.coffee

+      MnoeApiSvc.one('organizations', MnoeOrganizations.selectedId).one('/app_instances', instance.id).post('/create_omniauth', body)
+
+    @getSyncs = (instance) ->
+      MnoeApiSvc.one('organizations', MnoeOrganizations.selectedId).one('/app_instances', instance.id).one('/sync_history').get()


Do not use one without id parameter. You should use customGET or doGET.
cf. https://github.com/mgonto/restangular#custom-methods

alexnoox · 2017-10-10T06:50:03Z

src/app/views/apps/modals/app-connect-modal-cloud.html

+
+      <button class="btn btn-success" ng-click="synchronize(historicalData)" style="float: right;">Start synchronizing</button>
+
+      <div style="clear: both;"></div>


You can directly apply a clearfix class.

See how to use it here: https://getbootstrap.com/docs/3.3/css/#helper-classes-clearfix

alexnoox · 2017-10-10T06:50:49Z

src/app/views/apps/modals/app-connect-modal-cloud.html

+    <div class="modal-body">
+      <h4 class="text-center">Chose which entities you want to synchronize</h4>
+
+      <table>


Use bootstrap table classes: https://getbootstrap.com/docs/3.3/css/#tables

Remove corresponding LESS in app-connect-modal.less

alexnoox · 2017-10-10T06:51:54Z

src/app/views/apps/modals/app-settings-modal.coffee

+
+    MnoeAppInstances.getSyncs($scope.app)
+      .then((response) ->
+        for i in [0...response.length]


Use lodash map

alexnoox · 2017-10-10T06:53:40Z

src/app/views/apps/modals/app-settings-modal.coffee

-      app.add_on
+      app.add_on &&
+      app.organization &&
+      app.organization.has_account_linked


This code is already present somewhere, could this be in a service?

alexnoox · 2017-10-10T06:54:53Z

src/app/views/apps/modals/app-settings-modal.less

+        display:block;
+        clear:both;
+        content:'';
+    }


Either define a tab style globally or reuse an existing one.

manu-d · 2017-11-02T19:11:03Z

@alexnoox Please review. I'll rebase after to fix the conflicts

alexnoox · 2017-11-06T02:56:21Z

@manu-d Please rebase your branch to fix the conflicts

alexnoox

This is generally good. Just a few comments and remarks:

Make sure that all the locales are using a key
There is a typo in filename connec-addon-modal.coffee, it is missing a t in connect

alexnoox · 2017-11-06T03:02:56Z

src/app/components/connect-addon-modal/connec-addon-modal.coffee

+      MnoeAppInstances.submitForm(ctrl.app, ctrl.model)
+        .then((response) ->
+          if response.error
+            toastr.error(response.error)


Shouldn't the server send an error and this error be catched?

We have some validation on the connector side when a user wants to link their account.
For example, only one account can be linked, or sometimes, we know of some kind of common mistake people make. And so, for each of these situations, we have a custom error message that I returned in the response.
Should I change the json key from the response "error" to something like "linking_error"?

alexnoox · 2017-11-06T03:09:16Z

src/app/components/connect-addon-modal/connec-addon-modal.coffee

+      MnoeAppInstances.sync(ctrl.app, ctrl.historicalData)
+      ctrl.app.addon_organization.sync_enabled = true
+      ctrl.close()
+      toastr.success($filter('translate')("mno_enterprise.templates.components.addon_connect.sync.sync_launched"))


Instead of calling the filter in the controller, you should use the $translate service. Call it once at the initialisation to fetch all the locales, and then use the returned array to get the locale.

https://angular-translate.github.io/docs/#/api/pascalprecht.translate.$translate

alexnoox · 2017-11-06T03:10:13Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+    </ul>
+
+    <div class="modal-body">
+      <h4 class="text-center">Chose wether to synchronize historical data</h4>


Missing locale

alexnoox · 2017-11-06T03:11:35Z

src/app/components/connect-addon-modal/connec-addon-modal.coffee

+    ctrl.forceSelectEntities = ->
+      ctrl.hasChosenEntities = true
+
+    ctrl.update = (entities) ->


I don't think you need to pass the entities parameter. It is never used.

alexnoox · 2017-11-06T03:55:03Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+        <p translate="mno_enterprise.templates.components.addon_connect.sync.warning_hisorical_html" translate-values="{appname: $ctrl.app.name}"></p>
+        <input ng-model="$ctrl.historicalData" type="checkbox" display=inline-block></input>
+        <small ng-show="!$ctrl.historicalData"
+               translate="mno_enterprise.templates.components.addon_connect.sync.historical_unchecked" translate-values="{date: ($ctrl.date | amDateFormat:'dddd, MMMM Do YYYY, h:mm:ss a')}">


For the date format, please use a format supporting user's locale.

Cf.http://momentjs.com/

alexnoox · 2017-11-06T03:59:35Z

src/app/views/apps/modals/app-connect-modal.less

+
+  toast {
+    z-index: 9999;
+  }


Move this in index.less

alexnoox · 2017-11-06T04:01:52Z

src/app/views/apps/modals/app-settings-modal.coffee

+          types: data[1]
+          changeEntity: (model) ->
+            $scope.loadIdMaps(model)
+          getIndexFromValue: (value, array) ->


This could be more concise using lodash

alexnoox · 2017-11-10T00:30:55Z

@manu-d Please rebase this properly so we can only see only your changes.

alexnoox

@manu-d I did another review, a bit more granular. The code and logic are good generally.

Please see my comments and:

Write your then promise callbacks on one line
Please make sure everything supports i18n (especially in the tables)
Do not manage the buttons rendering in the controller, prefer using logic in the template

alexnoox · 2017-11-12T23:05:53Z

src/app/components/connect-addon-modal/connect-addon-modal.coffee

+      ctrl.app = ctrl.resolve.app
+      ctrl.hasLinked = ctrl.app.addon_organization.has_account_linked
+      MnoeAppInstances.getForm(ctrl.app)
+        .then((response) ->


Could be written on one line

alexnoox · 2017-11-12T23:06:57Z

src/app/components/connect-addon-modal/connect-addon-modal.coffee

+        )
+
+    ctrl.close = ->
+      ctrl.close()


Can't you just use $ctrl.close() in the template?
This is a bit redundant.

alexnoox · 2017-11-12T23:08:33Z

src/app/components/connect-addon-modal/connect-addon-modal.coffee

+      MnoeAppInstances.submitForm(ctrl.app, ctrl.model)
+        .then((response) ->
+          if response.error
+            toastr.error(response.error)


Shouldn't the error be catched in an error callback?

We have some validation on the connector side when a user wants to link their account.
For example, only one account can be linked, or sometimes, we know of some kind of common mistake people make. And so, for each of these situations, we have a custom error message that I returned in the response.
Should I change the json key from the response "error" to something like "linking_error"?

Yes, that would be great. This is a bit confusing and alarming. linking_error sounds more like a specific error.

alexnoox · 2017-11-12T23:08:54Z

src/app/components/connect-addon-modal/connect-addon-modal.coffee

+          else
+            ctrl.app.addon_organization.has_account_linked = true
+            ctrl.hasLinked = true
+          ctrl.isSubmitting = false


Should be in a finally callback.

alexnoox · 2017-11-12T23:09:29Z

src/app/components/connect-addon-modal/connect-addon-modal.coffee

+    ctrl.forceSelectEntities = ->
+      ctrl.hasChosenEntities = true
+
+    ctrl.update = (entities) ->


entities param is never used

alexnoox · 2017-11-12T23:48:16Z

src/app/views/apps/modals/app-settings-modal.coffee

+    $scope.selectHistory = ->
+      $scope.isLeftFooterButtonShown = true
+      $scope.selectedTab = 0
+      $scope.textForFooterButton = "Synchronize"


This is a rendering problematic. This should be in the template, using ng-show on buttons.

alexnoox · 2017-11-12T23:50:00Z

src/app/views/apps/modals/app-settings-modal.html

@@ -16,7 +16,54 @@
      <!-- Add on settings -->
      <div ng-show="helper.isAddOnSettingShown(app)">
        <hr>
-        <button ng-click="helper.addOnSettingLauch(app)" class="btn btn-warning">{{ 'mno_enterprise.templates.impac.dock.settings.access_add-on_settings' | translate }}</button>
+        <uib-tabset active="activeJustified" justified="true">
+          <uib-tab index="0" heading="History" ng-click="loadSyncs()" select="selectHistory()">


Heading i18n

alexnoox · 2017-11-12T23:50:07Z

src/app/views/apps/modals/app-settings-modal.html

+            </div>
+          </uib-tab>
+
+          <uib-tab index="1" heading="Data" select="selectData()">


Heading i18n

alexnoox · 2017-11-12T23:50:16Z

src/app/views/apps/modals/app-settings-modal.html

+            </div>
+          </uib-tab>
+
+          <uib-tab index="2" heading="Entities" select="selectEntities()">


Heading i18n

alexnoox · 2017-11-12T23:50:36Z

src/app/views/apps/modals/app-settings-modal.html

@@ -50,7 +97,19 @@
      <!-- Deletion -->
      <hr>
      <div ng-show="helper.canDeleteApp()" class="app-deletion">
-        <button class="btn btn-danger" ng-hide="app.showDelete" ng-click="app.showDelete = !app.showDelete">{{ 'mno_enterprise.templates.impac.dock.settings.delete' | translate }}</button>
+        <div class="col-xs-13">


This class doesn't exist

manu-d · 2017-11-15T18:01:33Z

@alexnoox
I18ned and rework the rendering problems. (It's true that it's much more readable like that ^^)

alexnoox

We are getting there 👍

Only a few remarks.

alexnoox · 2017-11-17T06:03:51Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+
+      <li ng-switch-when="true|1|2" ng-switch-when-separator="|"><a href="#" class="not-active">{{"mno_enterprise.templates.components.addon_connect.link_account.title" | translate}}</a></li>
+      <li ng-switch-when="true|1" ng-switch-when-separator="|">{{"mno_enterprise.templates.components.addon_connect.entities.select" | translate}}</li>
+      <li ng-switch-when="true|1" ng-switch-when-separator="|"><a href="#" ng-click="$ctrl.forceSelectEntities()">{{"mno_enterprise.templates.components.addon_connect.sync.start_sync" | translate}}</a></li>


In the ng-switch-when, could you only use numbers?

alexnoox · 2017-11-17T06:05:54Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+          <th>{{"mno_enterprise.templates.components.addon_connect.entities.to_connec" | translate:{ appname: $ctrl.app.app_name} }}&nbsp</th>
+        </tr>
+        <tr ng-repeat="entity in $ctrl.app.addon_organization.displayable_synchronized_entities">
+          <td>{{entity.connec_name}}&nbsp</td>


&nbsp to remove

alexnoox · 2017-11-17T06:06:01Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+
+      <table class="table-striped table-condensed" width="100%">
+        <tr>
+          <th>{{"mno_enterprise.templates.components.addon_connect.entities.entity" | translate}}&nbsp</th>


&nbsp to remove

alexnoox · 2017-11-17T06:06:09Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+      <table class="table-striped table-condensed" width="100%">
+        <tr>
+          <th>{{"mno_enterprise.templates.components.addon_connect.entities.entity" | translate}}&nbsp</th>
+          <th>{{"mno_enterprise.templates.components.addon_connect.entities.to_app" | translate:{ appname: $ctrl.app.app_name} }}&nbsp</th>


&nbsp to remove

alexnoox · 2017-11-17T06:06:14Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+        <tr>
+          <th>{{"mno_enterprise.templates.components.addon_connect.entities.entity" | translate}}&nbsp</th>
+          <th>{{"mno_enterprise.templates.components.addon_connect.entities.to_app" | translate:{ appname: $ctrl.app.app_name} }}&nbsp</th>
+          <th>{{"mno_enterprise.templates.components.addon_connect.entities.to_connec" | translate:{ appname: $ctrl.app.app_name} }}&nbsp</th>


&nbsp to remove

alexnoox · 2017-11-17T06:09:14Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+        translate-values="{appname: $ctrl.app.name}"></h3>
+  </div>
+
+


Remove double empty lines (only one)

alexnoox · 2017-11-17T06:09:19Z

src/app/components/connect-addon-modal/connect-addon-modal.html

+    </div>
+  </div>
+
+


Remove double empty lines (only one)

manu-d requested a review from alexnoox October 4, 2017 15:39

manu-d force-pushed the feature/application-setup branch from 881f4c5 to 0ef05a7 Compare October 4, 2017 15:47

alexnoox suggested changes Oct 10, 2017

View reviewed changes

manu-d force-pushed the feature/application-setup branch from d88f7e9 to adc13a3 Compare October 25, 2017 16:45

alexnoox suggested changes Nov 6, 2017

View reviewed changes

manu-d force-pushed the feature/application-setup branch from 6f23302 to 1b11694 Compare November 6, 2017 14:16

manu-d added the Status: Review Needed label Nov 7, 2017

alexnoox suggested changes Nov 12, 2017

View reviewed changes

manu-d force-pushed the feature/application-setup branch from 26df9cb to 132e984 Compare November 14, 2017 17:14

alexnoox suggested changes Nov 17, 2017

View reviewed changes

manu-d force-pushed the feature/application-setup branch from 1510f49 to 04d0f38 Compare November 22, 2017 10:03

alexnoox approved these changes Nov 26, 2017

View reviewed changes

manu-d force-pushed the feature/application-setup branch from 0ecb52b to 716b4ee Compare February 5, 2018 17:38

manu-d added 12 commits June 4, 2018 15:51

Add button to redirect to external app to create account

5cfe849

Fetch setup form from mno enterprise

069ea83

Rules to show buttons for app instance

5c788ae

Add modal to connect application

41adc85

Update modal to display addon-information

1d791df

Code review

077def1

Add smart table for syncs

e17f896

Fix method call

34a306b

Fetch and display idmaps

02207f5

Rework id_maps display

1416eaa

Move new modal to components

1bb720e

Remove unused files

9c22da1

manu-d added 13 commits June 4, 2018 15:58

Update condition to display addon settings

4d1ecc2

Add translations for app-connect-modal

3207bc2

Add translations for app settings

7f10c33

Add translation values for historical data

2ef5e38

Code review

719ce9d

I18n and update rendering logic

6ee24eb

Replace ternary operator and remove blank lines

69d4235

Ability to update synchronized entities

cb8e311

Update translation keys

af20db4

Update locales

7f7acd2

Add previous next when linking account

f1e6c93

Change representation of synced records

ea74635

Add modal to confirm disconnection

0087383

manu-d force-pushed the feature/application-setup branch from 716b4ee to 0087383 Compare June 4, 2018 15:38


		<button class="btn btn-success" ng-click="synchronize(historicalData)" style="float: right;">Start synchronizing</button>

		<div style="clear: both;"></div>

[APPINT-1044] Update frontend and components to display info about add-ons #352

Are you sure you want to change the base?

[APPINT-1044] Update frontend and components to display info about add-ons #352

Conversation

manu-d commented Oct 4, 2017 • edited Loading

manu-d commented Oct 4, 2017

alexnoox 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

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

manu-d commented Nov 2, 2017

alexnoox commented Nov 6, 2017

alexnoox 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

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

alexnoox commented Nov 10, 2017

alexnoox 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

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

manu-d commented Nov 15, 2017

alexnoox 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

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

manu-d commented Oct 4, 2017 •

edited

Loading