[MNOE-428] Migrate mno-enterprise to MnoHub API V2 #281

x4d3 · 2017-04-27T17:02:17Z

No description provided.

ouranos

1st review didn't go to the end as mentioned.

Something to be careful of is the check on organization role.
There was some logic to improve perfs: filter the Org array if loaded and otherwise load the single organization
We need to make sure we don't introduce major slow down on this part

ouranos · 2017-05-15T06:43:34Z

api/app/controllers/mno_enterprise/jpi/v1/admin/invites_controller.rb

      else
        status_scope = { status: 'accepted' }
      end
-      organization.org_invites.where(status_scope.merge(user_id: user.id)).first
+      Mno::Organization.includes(:user).where(status_scope.merge(user_id: user.id, organization_id: organization.id)).first


Shouldn't it be the Mno::OrgInvite?

Well spotted.

ouranos · 2017-05-15T06:48:38Z

api/app/controllers/mno_enterprise/jpi/v1/base_resource_controller.rb

+        @parent_organization ||= begin
+          id_or_uid = params[:organization_id]
+          field = is_integer?(id_or_uid)? :id : :uid
+          Mno::Organization.includes(:orga_relations, :users).find(field => id_or_uid).first


This is no longer scoped to the current_user?

True, needs to find a way to do it.

ouranos · 2017-05-15T06:51:53Z

api/app/views/mno_enterprise/jpi/v1/organizations/_member.json.jbuilder

-  json.role member.role
-elsif member.is_a?(MnoEnterprise::OrgInvite)
+  json.role organization.role(member)
+elsif member.is_a?(Mno::OrgaInvite)


MnoEnterprise::OrgInvite -> Mno::OrgaInvite?

yes. It is OrgaInvite in MnoHub. Let's have consistency :)

ouranos · 2017-05-15T06:58:33Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb

@@ -13,31 +13,34 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::AppInstancesController
  #==================================================================
  # Instance methods
  #==================================================================
-  # GET /mnoe/jpi/v1/organization/1/apps.json?timestamp=151452452345
+  # GET /mnoe/jpi/v1/organization/1/apps.json?tg=151452452345


I think we can deprecate the timestamp filtering.

This was used in old versions of the frontend when we were displaying the status of hosted apps.
There was a poller that was sending the current TS and it was used to change the status from starting to running.
This has been removed a while back.

ok. will do

Done. But you should update the comment :)

ouranos · 2017-05-15T07:00:38Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb

  end

  # DELETE /mnoe/jpi/v1/app_instances/1
  def destroy
-    app_instance = MnoEnterprise::AppInstance.find(params[:id])
+    app_instance = Mno::AppInstance.find(params[:id])[0]


find is always returning an array? Unlike AR/Her which distinguish find from find_all?
In this case, I'd prefer first than [0]

Or should we use find_one?

yes, everything is a query for him, and therefore find return an array (I am not totally agains the approach, if you consider that everything is a collection, you can factorise code)

It is explained here: JsonApiClient/json_api_client#75

we should indeed use my lovely find_one

ouranos · 2017-05-15T07:03:41Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/impac/dashboards_controller.rb

@@ -95,6 +95,7 @@ def dashboard_params
        whitelisted[:settings] = params[:dashboard][:metadata] || {}
      end
      .except(:metadata)
+      .merge(owner_type: "User", owner_id: current_user.id)


This will cause backward compatibility issues with some customer projects where the owner is an Org.
They've extended the dashboards method.
I can share the code customisation with you as I believe it's quite flexible.

Not to much of an issue as we'll tag a major release for this

That'll likely be a problem in the feature so it'd be good to address it.

Is that something that could be fixed easily?

we could change the api to accept an owner_type externally, that could be User or Organization

but the current code was using UserDashboard, and therefore was already creating a dashboard for which the owner was a user.

There is no backward compatibility issue.

ouranos · 2017-05-15T07:07:55Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/impac/widgets_controller.rb

+    return render_not_found('widget') unless widget
+    MnoEnterprise::EventLogger.info('widget_delete', current_user.id, 'Widget Deletion', widget)
+    widget.destroy
+    if widget.errors.empty?


Could we have the logger in this block? Or do you put it before to avoid issue with its being deleted?

You got it right. widget.id is empty after you call destroy. Quite annoying actually.

ouranos · 2017-05-15T07:13:06Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/organizations_controller.rb

-        organization.users.to_a.find { |u| u.email == email } ||
-          organization.org_invites.active.where(user_email: email).first
+        organization.users.find { |u| u.email == email } ||
+        organization.orga_invites.find { |u| u.status == 'pending' && u.user_email = email}


u.user_email == email

ouranos

Few general notes:

Change the Mno namespace to MnoEnterprise
Few TODO left
Perf improvements? Do you see any improvements in the amount and size of request to MnoHub?
stub_audit_events globally
I'm dropping support to ruby < 2.3 in the 4.0 branch so feel free to leverage Hash#dig and & (don't go overboard ;))

ouranos · 2017-06-14T05:33:32Z

api/app/views/mno_enterprise/jpi/v1/app_instances/_resource.json.jbuilder

 json.created_at app_instance.created_at

-json.per_user_licence app_instance.per_user_licence


fields removed?

Yes, it did not appear to be used in mno-enterprise-angular.

Yes but this is because the PR is still in review/revision: maestrano/mno-enterprise-angular#82

ouranos · 2017-06-14T05:37:27Z

api/lib/mno_enterprise/audit_events_listener.rb

@@ -2,31 +2,23 @@

 module MnoEnterprise
  class AuditEventsListener
-    include HTTParty
-    base_uri "#{MnoEnterprise.mno_api_private_host || MnoEnterprise.mno_api_host}/api/mnoe/v1/audit_events"
-    read_timeout 0.1


Could we still have a short read timeout here? As we don't care about the response.
Not a biggie as the event logging is done through ActiveJob anyway.

ouranos · 2017-06-14T05:38:26Z

api/lib/mno_enterprise/concerns/controllers/deletion_requests_controller.rb

@@ -68,9 +68,9 @@ def freeze_account
        # Check that the deletion_request has the right status
        if @deletion_request.status == 'pending'
          @deletion_request.freeze_account!
-          format.html { redirect_to @deletion_request, notice: 'Your account has been frozen' }
+          format.html { redirect_to({action: :show, id: @deletion_request.id}, notice: 'Your account has been frozen') }


Interesting, the previous code is no longer working?

No, but only because of the difference of namespace between the controller and the model. This should be fixed if I put everything on the same namespace.

Which namespace? The Mno one? That should be working now ;)

ouranos · 2017-06-14T05:39:26Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb

@@ -13,31 +13,34 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::AppInstancesController
  #==================================================================
  # Instance methods
  #==================================================================
-  # GET /mnoe/jpi/v1/organization/1/apps.json?timestamp=151452452345
+  # GET /mnoe/jpi/v1/organization/1/apps.json?tg=151452452345


Done. But you should update the comment :)

ouranos · 2017-06-14T05:40:25Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb

+    if app_instance.errors.any?
+      render json: app_instance.errors, status: :bad_request
+    else
+      MnoEnterprise::EventLogger.info('app_add', current_user.id, 'App added', app_instance.first)


app_instance.first?
It's not super intuitive to have provision_app_instance return an array

I agree, but this is the way of working of json_api_client, everything returned is always an array.

ouranos · 2017-06-14T06:25:58Z

core/app/models/mno/user.rb

+    # Validation
+    #================================
+    #
+    # if Devise.password_regex


Validation?

ouranos · 2017-06-14T06:27:07Z

core/lib/her_extension/middleware/mnoe_api_v1_parse_json.rb

-          return Time.iso8601(res)
-        else
-          return res
+          when res.kind_of?(Array)


Indent when as deep as case 👮‍♂️

Given that's Her related stuff, I'm sure it's Rubymine :)

ouranos · 2017-06-14T06:28:40Z

core/lib/json_api_client_extension/json_api_client_orm_adapter.rb

+    # @see OrmAdapter::Bax``
+    def get(id)
+      res = klass.includes(:deletion_requests, :organizations, :orga_relations, :dashboards).find(wrap_key(id))
+      if(res.errors && res.errors.first && res.errors.first.code != "404")


See my ruby 2.3 comment, & ftw!

ouranos · 2017-06-14T06:29:40Z

core/lib/mno_enterprise/core.rb

+  @@mno_api_v2_root_path = "/api/mnoe/v2"
+
+  mattr_reader :mnoe_api_v2
+  @@mnoe_api_v2 = nil


What is this one used for?

Nothing, good spot.

ouranos · 2017-06-14T06:31:13Z

core/mno-enterprise-core.gemspec

@@ -47,6 +47,8 @@ Gem::Specification.new do |s|
  # Config files per environment
  s.add_dependency 'config', '~> 1.0', '< 1.3'

+  s.add_dependency 'json_api_client', '~> 1.3'


Add it next to her and faraday

ouranos

Another review.

Please do a separate commit for this so it's easier to review and it won't crash the github diff ;)

general comments:

still lots of TODO left
not a big fan of the let!(stub) is there a particular reason?
remove her dependency (and all helpers files)?
AppReview/AppComment => Needed for Opal project

question:

Schema information in model is quite useful?
attributes to property is not 1 => 1 (eg: a big list is replaced by created_at/updated_at)
the assocations are no longer explicit?

I'll merge it soon (ie tomorrow) in a v4 branch and we can do more PR to iterate

ouranos · 2017-06-19T05:41:02Z

api/app/views/mno_enterprise/jpi/v1/app_instances/_resource.json.jbuilder

 json.created_at app_instance.created_at

-json.per_user_licence app_instance.per_user_licence


Yes but this is because the PR is still in review/revision: maestrano/mno-enterprise-angular#82

ouranos · 2017-06-19T05:45:14Z

api/app/views/mno_enterprise/jpi/v1/audit_events/index.json.jbuilder

@@ -1,4 +1,6 @@
 json.audit_events do
  json.array! @audit_events, partial: 'audit_event', as: :audit_event
 end
-json.metadata @audit_events.metadata
+# TODO: Port previous pagination metadata information ?
+# json.metadata @audit_events.metadata


Is this used somewhere? I couldn't find it in the frontend
Seems that the pagination is done through headers

Ok. Removing it.

ouranos · 2017-06-19T05:49:05Z

api/app/views/mno_enterprise/jpi/v1/organizations/_current_user.json.jbuilder

@@ -2,4 +2,4 @@ json.id user.id
 json.name user.name
 json.surname user.surname
 json.email user.email
-json.role organization.members.to_a.find { |e| e.id == user.id }.role
+json.role user.role(organization)


There are a couple calls like this. What is the effect in terms of performance?
This was introduced for sme-platform release to improve performance by avoiding extra calls to MnoHub.

There is no performance issue, as the user is already loaded with it's organization.

ouranos · 2017-06-19T05:50:19Z

api/lib/mno_enterprise/concerns/controllers/deletion_requests_controller.rb

@@ -68,9 +68,9 @@ def freeze_account
        # Check that the deletion_request has the right status
        if @deletion_request.status == 'pending'
          @deletion_request.freeze_account!
-          format.html { redirect_to @deletion_request, notice: 'Your account has been frozen' }
+          format.html { redirect_to({action: :show, id: @deletion_request.id}, notice: 'Your account has been frozen') }


Which namespace? The Mno one? That should be working now ;)

ouranos · 2017-06-19T05:59:06Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/impac/dashboards_controller.rb

@@ -95,6 +95,7 @@ def dashboard_params
        whitelisted[:settings] = params[:dashboard][:metadata] || {}
      end
      .except(:metadata)
+      .merge(owner_type: "User", owner_id: current_user.id)


Is that something that could be fixed easily?

ouranos · 2017-06-19T07:21:06Z

core/lib/json_api_client_extension/validations/remote_uniqueness_validation.rb

+  module Validations
+    class RemoteUniquenessValidator < ::ActiveModel::EachValidator
+      def validate_each(record, attribute, value)
+        list = record.class.where({attribute => value}).paginate(page: 1, per_page: 1).to_a


can you do record.class.find_first({attribute => value}).to_a ?

and list.any? { |e| e && e.id != record.id }

I am not sure find_first exists. I will have a look.

find_first does not exist in json api client. :(

ouranos · 2017-06-19T07:25:07Z

core/lib/mno_enterprise/concerns/models/app_instance.rb

-
-  # Return true if the instance can be considered active
-  # Route53 DNS propagation may take up to a minute, so we force a minimum of 60 seconds before considering the application online
-  def active?


Shouldn't we keep all those instances methods (this and the one below)

If they are not called. (and they are not), why should we ?

https://github.com/maestrano/mno-enterprise/blob/master/api/app/views/mno_enterprise/jpi/v1/app_instances/_resource.json.jbuilder#L11

ouranos · 2017-06-19T07:26:34Z

core/lib/mno_enterprise/concerns/models/team.rb

@@ -42,26 +23,4 @@ module ClassMethods
  #==================================================================
  # Instance methods
  #==================================================================
-  # Add a user to the team
-  # TODO: specs
-  def add_user(user)


This is no longer needed?

Needs to check, good spot.

It is used in organization_controller.

ouranos · 2017-06-19T07:27:22Z

core/lib/mno_enterprise/core.rb

@@ -22,8 +22,10 @@
 require "her_extension/middleware/mnoe_raise_error"


Let's get rid of this!

ouranos · 2017-06-19T07:29:11Z

core/mno-enterprise-core.gemspec

@@ -21,6 +21,7 @@ Gem::Specification.new do |s|
  s.add_dependency "her", "~> 0.7.3"


Let's remove it!

x4d3 · 2017-06-19T08:20:44Z

Some of the TODO were already here, (for example the Ability on the Impac model)

not a big fan of the let!(stub) is there a particular reason?

To be able to replace the stub for some particular case.

remove her dependency (and all helpers files)?

Yep, I should do that in a separate commit indeed.
AppReview/AppComment => Needed for Opal project

Yes, I had issue with polymorphism.

question:

Schema information in model is quite useful?

True, however I am not a big fan as it is not always up to date. I think I would prefer to use the attributes as a way to document the fields and their types. What do you think ?

attributes to property is not 1 => 1 (eg: a big list is replaced by created_at/updated_at)

Yes, because they are not needed explicitly to work. I will add them (Maybe using MnoHub)

the assocations are no longer explicit?

Unfortunately no, it is documented in MnoHub, but not visible in MnoE,

ouranos · 2017-06-20T00:02:52Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/organizations_controller.rb

      # Admin cannot edit Super Admin
-      raise CanCan::AccessDenied if (member.is_a?(MnoEnterprise::User) && organization.role(member) == 'Super Admin') || (member.is_a?(MnoEnterprise::OrgaInvite) && member.user_role == 'Super Admin')
-    elsif member.id == current_user.id && attributes[:role] != 'Super Admin' && organization.orga_relations.count {|u| u.role == 'Super Admin'} <= 1
+      if member_current_role == 'Super Admin'


This can now be done in one line with if as a modifier instead of a statement

ouranos · 2017-06-20T00:03:59Z

api/spec/controllers/mno_enterprise/deletion_requests_controller_spec.rb

@@ -58,14 +58,13 @@ def main_app

      context 'when the request is pending' do
        it 'freezes the account' do
-          expect(controller.current_user).to receive(:current_deletion_request).and_return(deletion_req)
-          expect(deletion_req).to receive(:freeze_account!)
+          expect_any_instance_of(MnoEnterprise::DeletionRequest).to receive(:freeze_account!)


I think rubocop prefers the previous code :)
Because you're testing you're freezing the correct deletion request

but it won't be the same object, as the one passed is a one created from the parsing of the api result...

- Add missing methods on user and organization - User -- Add create_api_credentials - Organization -- add role -- add payment_restriction -- add orga_relation - KPI and Alert - Implement Invite and Delete Organization Members - Implement Create Organization - Implement Update member - Implement Team management - Add star_ready? connec_ready? and responsive - Implement app instance provisioning - Implement Team Update - Implement Dashboard management - Implement widget controller - Update Dashboard - Correct issues with Dashboard's widgets not being sorted - Implement App Synchronisation - Apply Code Review Remark - Implement Audit Logging - Start Spec Writing - Adapt organization_spec.rb - Adapt Team Controller - Adapt Specs - Update Remote Authenticable - Remove Her Specifics Specs - Adapt App Specs - Adapt Model Specs - Adapt Confirmation Controller - Adapt DeletionRequestController KpisController - Adapt Marketplace Controller Spec and IntercomEventsListern - Adapt Current User Controller Spec - Apply Code review remarks - Add to_audit_event - Adapt Toto

- add per_user_licence back - extract custom_parser to a file - removed or corrected comments - simplified code

- Manage App Shared Entities - Manage Widget KPIS - Fix issue with user's role - Fix App instance querying - Fix error while deleting Teams - Fix error when an metadata containing a time was sent to the EventLogger - Remove logging when not in dev

ouranos

Quick question. Why create AppSharedEntity and not reuse SharedEntity?

ouranos · 2017-06-22T00:29:20Z

api/app/views/mno_enterprise/jpi/v1/marketplace/_app.json.jbuilder

-  json.array! app.shared_entities do |shared_entity|
-    json.extract! shared_entity, :shared_entity_nid, :shared_entity_name, :write, :read
+if app.app_shared_entities.any?
+  json.app_shared_entities do


The response is changing from shared_entities to app_shared_entities?

The app contains a list of app_shared_entities in the v2 api.

ouranos · 2017-06-22T00:30:10Z

api/app/views/mno_enterprise/jpi/v1/marketplace/_app.json.jbuilder

+if app.app_shared_entities.any?
+  json.app_shared_entities do
+    json.array! app.app_shared_entities do |shared_entity|
+      json.shared_entity_nid shared_entity.shared_entity.nid


Why not use json.extract!? It's a bit more concise?

because it's not taking the fields name by name, it is mapping
shared_entity_nid to shared_entity.nid.

x4d3 · 2017-06-22T07:36:22Z

because in the MnoHub model, an app contains AppSharedEntities, that contains SharedEntity

x4d3 added the Status: In Progress label Apr 27, 2017

x4d3 requested a review from ouranos April 27, 2017 17:02

ouranos reviewed May 15, 2017

View reviewed changes

x4d3 force-pushed the feature/api-v2 branch 2 times, most recently from 29559a2 to 6e422a4 Compare May 24, 2017 16:44

ouranos added this to the v4.0 milestone Jun 7, 2017

x4d3 force-pushed the feature/api-v2 branch from 7fdb566 to 094bbb3 Compare June 7, 2017 13:38

ouranos reviewed Jun 14, 2017

View reviewed changes

x4d3 force-pushed the feature/api-v2 branch 3 times, most recently from 05193b8 to 3b59dc8 Compare June 14, 2017 16:46

x4d3 changed the title ~~[WIP] Mnoe App V2 Development~~ [MNOE-428] Migrate mno-enterprise to MnoHub API V2 Jun 14, 2017

x4d3 added Status: Review Needed and removed Status: In Progress labels Jun 14, 2017

x4d3 closed this Jun 19, 2017

x4d3 deleted the feature/api-v2 branch June 19, 2017 07:07

x4d3 restored the feature/api-v2 branch June 19, 2017 07:09

x4d3 reopened this Jun 19, 2017

ouranos reviewed Jun 19, 2017

View reviewed changes

ouranos reviewed Jun 20, 2017

View reviewed changes

ouranos changed the base branch from master to 4.0 June 20, 2017 08:29

x4d3 added 3 commits June 20, 2017 10:25

Make frontend spec working

53dd9dd

Apply code review suggestions

81c447b

- add per_user_licence back - extract custom_parser to a file - removed or corrected comments - simplified code

x4d3 force-pushed the feature/api-v2 branch from 8f2c83e to 81c447b Compare June 20, 2017 09:26

x4d3 added 2 commits June 20, 2017 12:03

Adapt Specs after rebase to 4.0

3f96075

Fix several issues

1555354

- Manage App Shared Entities - Manage Widget KPIS - Fix issue with user's role - Fix App instance querying - Fix error while deleting Teams - Fix error when an metadata containing a time was sent to the EventLogger - Remove logging when not in dev

ouranos reviewed Jun 22, 2017

View reviewed changes

ouranos merged commit eb2546f into maestrano:4.0 Jun 22, 2017

		json.created_at app_instance.created_at

		json.per_user_licence app_instance.per_user_licence

		@@ -22,8 +22,10 @@
		require "her_extension/middleware/mnoe_raise_error"

		@@ -21,6 +21,7 @@ Gem::Specification.new do \|s\|
		s.add_dependency "her", "~> 0.7.3"

[MNOE-428] Migrate mno-enterprise to MnoHub API V2 #281

[MNOE-428] Migrate mno-enterprise to MnoHub API V2 #281

Conversation

x4d3 commented Apr 27, 2017

ouranos 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

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

x4d3 Jun 19, 2017 • edited Loading

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

ouranos 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

x4d3 Jun 14, 2017 • edited Loading

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

ouranos 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

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

x4d3 commented Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouranos 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

x4d3 commented Jun 22, 2017

x4d3 Jun 19, 2017 •

edited

Loading

x4d3 Jun 14, 2017 •

edited

Loading

x4d3 commented Jun 19, 2017 •

edited

Loading