Apply code review suggestions

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

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

@@ -5,6 +5,7 @@ json.name app_instance.name
 json.status app_instance.status
 json.oauth_keys_valid app_instance.oauth_keys_valid
 json.created_at app_instance.created_at
+json.per_user_licence app_instance.per_user_licence
 
 if app_instance.oauth_company
   json.oauth_company_name app_instance.oauth_company

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

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

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({action: :show, id: @deletion_request.id}, notice: 'Your account has been frozen') }
+          format.html { redirect_to @deletion_request, notice: 'Your account has been frozen' }
         else
-          format.html { redirect_to({action: :show, id: @deletion_request.id}, alert: 'Invalid action')}
+          format.html { redirect_to @deletion_request, alert: 'Invalid action' }
         end
       else
         format.html { redirect_to main_app.root_path, alert: 'This deletion request is invalid or expired' }
@@ -95,9 +95,9 @@ def checkout
           #   Finally Perform the checkout
           @deletion_request.status = 'account_checked_out'
           @deletion_request.save
-          format.html { redirect_to({action: :show, id: @deletion_request.id}, notice: 'Checkout has been performed successfully')}
+          format.html { redirect_to @deletion_request, notice: 'Checkout has been performed successfully' }
         else
-          format.html { redirect_to({action: :show, id: @deletion_request.id}, alert: 'Invalid action') }
+          format.html { redirect_to @deletion_request, alert: 'Invalid action' }
         end
       else
         format.html { redirect_to main_app.root_path, alert: 'This deletion request is invalid or expired' }

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

@@ -9,7 +9,7 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::OrganizationsController
   included do
     respond_to :json
     before_filter :organization_management_enabled?, only: [:create, :update, :destroy, :update_billing,
-                                                           :invite_members, :update_member, :remove_member]
+                                                            :invite_members, :update_member, :remove_member]
   end
 
   #==================================================================
@@ -57,7 +57,7 @@ def create
     # Create new organization
     @organization = MnoEnterprise::Organization.create(organization_update_params)
     # Add the current user as Super Admin
-    @organization.add_user(current_user,'Super Admin')
+    @organization.add_user(current_user, 'Super Admin')
     # Bust cache
     current_user.refresh_user_cache
 
@@ -84,7 +84,7 @@ def update_billing
   # PUT /mnoe/jpi/v1/organizations/:id/invite_members
   def invite_members
     # Filter
-    whitelist = ['email','role','team_id']
+    whitelist = ['email', 'role', 'team_id']
     attributes = []
     params[:invites].each do |invite|
       attributes << invite.slice(*whitelist)
@@ -120,10 +120,16 @@ def update_member
     if current_user.role(organization) == 'Admin'
       # Admin cannot assign Super Admin role
       raise CanCan::AccessDenied if attributes[:role] == 'Super Admin'
-
+      member_current_role = if member.is_a?(MnoEnterprise::User)
+                              organization.role(member)
+                            elsif member.is_a?(MnoEnterprise::OrgaInvite)
+                              member.user_role
+                            end
       # 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'
+        raise CanCan::AccessDenied
+      end
+    elsif member.id == current_user.id && attributes[:role] != 'Super Admin' && organization.orga_relations.count { |u| u.role == 'Super Admin' } <= 1
       # A super admin cannot modify his role if he's the last super admin
       raise CanCan::AccessDenied
     end
@@ -161,47 +167,47 @@ def remove_member
   end
 
   protected
-    def member
-      @member ||= begin
-        email = params.require(:member).require(:email)
-        # Organizations are already loaded with all users
-        organization.users.find { |u| u.email == email } ||
-        organization.orga_invites.find { |u| u.status == 'pending' && u.user_email == email}
-      end
+  def member
+    @member ||= begin
+      email = params.require(:member).require(:email)
+      # Organizations are already loaded with all users
+      organization.users.find { |u| u.email == email } ||
+        organization.orga_invites.find { |u| u.status == 'pending' && u.user_email == email }
     end
+  end
 
-    def organization
-      @organization ||= MnoEnterprise::Organization.find_one(params[:id], :users, :orga_invites, :orga_relations, :credit_card)
-    end
+  def organization
+    @organization ||= MnoEnterprise::Organization.find_one(params[:id], :users, :orga_invites, :orga_relations, :credit_card)
+  end
 
-    def organization_permitted_update_params
-      [:name, :soa_enabled, :industry, :size]
-    end
+  def organization_permitted_update_params
+    [:name, :soa_enabled, :industry, :size]
+  end
 
-    def organization_update_params
-      params.fetch(:organization, {}).permit(*organization_permitted_update_params)
-    end
+  def organization_update_params
+    params.fetch(:organization, {}).permit(*organization_permitted_update_params)
+  end
 
-    def organization_billing_params
-      params.require(:credit_card).permit(
-        'title', 'first_name', 'last_name', 'number', 'month', 'year', 'country', 'verification_value',
-        'billing_address', 'billing_city', 'billing_postcode', 'billing_country'
-      )
-    end
+  def organization_billing_params
+    params.require(:credit_card).permit(
+      'title', 'first_name', 'last_name', 'number', 'month', 'year', 'country', 'verification_value',
+      'billing_address', 'billing_city', 'billing_postcode', 'billing_country'
+    )
+  end
 
-    def check_valid_payment_method
-      return true unless organization.payment_restriction.present?
+  def check_valid_payment_method
+    return true unless organization.payment_restriction.present?
 
-      if CreditCardValidations::Detector.new(organization_billing_params[:number]).valid?(*organization.payment_restriction)
-        true
-      else
-        cards = organization.payment_restriction.map(&:capitalize).to_sentence
-        @credit_card.errors.add(:number, "Payment is limited to #{cards} Card Holders")
-        false
-      end
+    if CreditCardValidations::Detector.new(organization_billing_params[:number]).valid?(*organization.payment_restriction)
+      true
+    else
+      cards = organization.payment_restriction.map(&:capitalize).to_sentence
+      @credit_card.errors.add(:number, "Payment is limited to #{cards} Card Holders")
+      false
     end
+  end
 
-    def organization_management_enabled?
-      return head :forbidden unless Settings.organization_management.enabled
-    end
+  def organization_management_enabled?
+    return head :forbidden unless Settings.organization_management.enabled
+  end
 end

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

@@ -100,7 +100,7 @@ def update_members(action)
   def update_params
     update = params.require(:team).permit(:name)
     if params[:team] && params[:team][:app_instances]
-      list = params[:team][:app_instances].select { |e| e != {} }.map { |e| e['id'] }
+      list = params[:team][:app_instances].map { |e| e['id'] }.compact
       update[:app_instance_ids] = list
     end
     update

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

@@ -31,9 +31,11 @@ def show
       @org_invite = MnoEnterprise::OrgaInvite.includes(:user, :organization).where(id: params[:id], token: params[:token], status: 'pending').first
       if @org_invite && !@org_invite.expired? && @org_invite.accept!(current_user)
         redirect_path = add_param_to_fragment(redirect_path.to_s, 'dhbRefId', @org_invite.organization.id)
+        # TODO: Add i18n
         message = { notice: "You are now part of #{@org_invite.organization.name}" }
         yield(:success, @org_invite) if block_given?
       elsif @org_invite && @org_invite.expired?
+        # TODO: Add i18n
         message = { alert: "It looks like this invite has expired. Please ask your company administrator to resend the invite." }
       end
     end

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!)
           subject
         end
 
         it 'redirects to the deletion request' do
           subject
-          expect(response).to redirect_to(deletion_request_url(deletion_req.id))
+          expect(response).to redirect_to(deletion_request_url(deletion_req))
         end
       end
 
@@ -79,7 +78,7 @@ def main_app
 
         it 'redirects to the deletion request' do
           subject
-          expect(response).to redirect_to(deletion_request_url(deletion_req.id))
+          expect(response).to redirect_to(deletion_request_url(deletion_req))
         end
 
         it 'displays an error message' do
@@ -112,7 +111,7 @@ def main_app
 
         it 'redirects to the deletion request' do
           subject
-          expect(response).to redirect_to(deletion_request_url(deletion_req.id))
+          expect(response).to redirect_to(deletion_request_url(deletion_req))
         end
 
         it 'displays an error message' do

api/spec/controllers/mno_enterprise/jpi/v1/app_instances_controller_spec.rb

@@ -58,13 +58,15 @@ module MnoEnterprise
       let(:app) { build(:app, nid: 'my-app') }
       let(:app_instance) { build(:app_instance, app: app, owner: organization, owner_id: organization.id) }
       subject { post :create, organization_id: organization.id, nid: 'my-app' }
-
+      it_behaves_like 'jpi v1 protected action'
       before do
         stub_api_v2(:post, '/app_instances/provision', app_instance)
+        sign_in user
       end
-      it_behaves_like 'jpi v1 protected action'
+
       it {
-        subject
+        expect(subject).to be_successful
+        assert_requested_api_v2(:post, '/app_instances/provision')
       }
     end
 

api/spec/controllers/mno_enterprise/jpi/v1/impac/kpis_controller_spec.rb

@@ -77,10 +77,10 @@ module MnoEnterprise
     describe 'POST #create' do
       shared_examples "create kpi action" do
 
-        it "creates the kpi" do
+        xit "creates the kpi" do
           subject
           # TODO: check that the rendered kpi is the created one
-          # expect(assigns(:kpi)).to eq(kpi)
+          expect(assigns(:kpi)).to eq(kpi)
         end
 
         context "when there are kpi targets" do
@@ -90,10 +90,10 @@ module MnoEnterprise
             stub_api_v2(:post, "/alerts", alert)
           end
 
-          it "creates kpi alerts" do
+          xit "creates kpi alerts" do
             subject
             # TODO: Check that the alerts are rendered
-            # expect(assigns(:kpi).alerts).to eq([alert])
+            expect(assigns(:kpi).alerts).to eq([alert])
             expect(response).to have_http_status(:ok)
           end
         end
@@ -140,10 +140,10 @@ module MnoEnterprise
 
         it_behaves_like 'create kpi action'
 
-        it '.widget retrieves the correct widget' do
+        xit '.widget retrieves the correct widget' do
           subject
           # TODO: check that the widget is well rendered
-          #expect(assigns(:widget)).to eq(widget)
+          expect(assigns(:widget)).to eq(widget)
         end
       end
     end

core/app/models/mno_enterprise/base_resource.rb

@@ -1,39 +1,9 @@
 require 'json_api_client'
 module MnoEnterprise
-
-  class CustomParser < ::JsonApiClient::Parsers::Parser
-    def self.parameters_from_resource(params)
-      hash = super
-      parse_types(hash)
-    end
-
-    def self.parse_types(res)
-      case res
-      when Array
-        return res.map { |e| parse_types(e) }
-      when Hash
-        if res.key?('cents') && res.key?('currency')
-          return Money.new(res['cents'], res['currency'])
-        else
-          hash = res.dup
-          hash.each do |k, v|
-            hash[k] = parse_types(v)
-          end
-          return hash
-        end
-      when String
-        if res =~ /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/i
-          return Time.iso8601(res)
-        end
-      end
-      res
-    end
-  end
-
   class BaseResource < ::JsonApiClient::Resource
     include ActiveModel::Callbacks
     self.site = URI.join(MnoEnterprise.api_host, MnoEnterprise.mno_api_v2_root_path).to_s
-    self.parser = CustomParser
+    self.parser = JsonApiClientExtension::CustomParser
 
     define_callbacks :update
     define_callbacks :save

core/app/models/mno_enterprise/user.rb

@@ -71,14 +71,9 @@ def initialize(params = {})
     # Return nil in case of failure
     def self.authenticate_user(auth_hash)
       result = self.authenticate({data: {attributes: auth_hash}})
-      if result
-        u = result.first
-        if u && u.id
-          # u.clear_attribute_changes!
-          return u
-        end
+      if (u = result&.first) && u.id
+        u
       end
-      nil
     rescue JsonApiClient::Errors::NotFound
 
     end

core/lib/devise/models/remote_authenticatable.rb

@@ -12,7 +12,7 @@ module RemoteAuthenticatable
       # If the authentication fails you should return false
       #
       def remote_authentication(authentication_hash)
-        self.class.authenticate_user(authentication_hash) # call MnoEnterprise::User.authenticate
+        self.class.authenticate_user(authentication_hash)
       end
 
       included do
@@ -28,7 +28,6 @@ def send_password_change_notification
       # Overriden methods from Devise::Models::Authenticatable
       ####################################
       module ClassMethods
-
         # Flag to enable password change notification
         Devise::Models.config(self, :send_password_change_notification)
 

core/lib/her_extension/middleware/mnoe_api_v1_parse_json.rb

@@ -19,20 +19,20 @@ def parse(body)
 
       def parse_types(res)
         case
-          when res.kind_of?(Array)
-            return res.map { |e| parse_types(e) }
-          when res.kind_of?(Hash) && res[:cents] && res[:currency]
-            Money.new(res[:cents], res[:currency])
-          when res.kind_of?(Hash)
-            hash = res.dup
-            hash.each do |k, v|
-              hash[k] = parse_types(v)
-            end
-            return hash
-          when res.is_a?(String) && res =~ /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/i
-            return Time.iso8601(res)
-          else
-            return res
+        when res.kind_of?(Array)
+          return res.map { |e| parse_types(e) }
+        when res.kind_of?(Hash) && res[:cents] && res[:currency]
+          Money.new(res[:cents], res[:currency])
+        when res.kind_of?(Hash)
+          hash = res.dup
+          hash.each do |k, v|
+            hash[k] = parse_types(v)
+          end
+          return hash
+        when res.is_a?(String) && res =~ /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/i
+          return Time.iso8601(res)
+        else
+          return res
         end
       end