[MNOE-688] Generate getter and setter for has_one relationship #560

x4d3 · 2017-11-01T17:56:12Z

This PR introduces 2 big changes.

The use of a has_one relationship helper
Stop loading everything in current_user

has_one relationship helper

This PR introduces an helper to easily create elements with relationships.

o = OrgaRelation.new(role: 'Super Admin')
o.relationships.user = User.new(id: user_id)
o.relationships.organization = User.new(id: organization_id)
o.save!

is replaced by

OrgaRelation.create!(user: user, organization: organization, role: 'Super Admin')

make current_user call smaller

In the current version, current_user is preloaded with a lot of dependencies, which are not always used. This can introduce problems of performances, caching and pagination. The objective of this PR is too stop relying on the autoload of theses dependencies and let the controllers manage the loading of what they need.

ouranos · 2017-11-02T07:32:52Z

core/lib/json_api_client_extension/has_one_extension.rb

+  class_methods do
+    def has_one(attr_name, options = {})
+      class_eval <<-CODE
+        def #{attr_name}_id=(id)


Coud you add something like:

ActiveSupport::Deprecation.warn('Use relationships instead') # => "DEPRECATION WARNING: Use relationships instead (called from your_code.rb:1)"

This will allow us to spot and progressively replace all the calls

manu-d · 2017-11-15T10:00:34Z

api/app/controllers/mno_enterprise/jpi/v1/admin/impac/dashboard_templates_controller.rb

-    def dashboard_template
-      @dashboard_template ||= dashboard_templates.find(params[:id].to_i).first
+    def load_organizations
+      @organizations = MnoEnterprise::Organization.where('user.ids': current_user.id)


@organizations = MnoEnterprise::Organization.where('users.id': current_user.id)

manu-d · 2017-11-15T10:07:47Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/admin/base_resource_controller.rb

  def parent_organization
-    @parent_organization ||= current_user.organizations.to_a.find { |o| o.id.to_s == params[:organization_id].to_s }
+    @parent_organization ||= MnoEnterprise::Organization.where(user_id: current_user.id, id: params[:organization_id]).first


@parent_organization ||= MnoEnterprise::Organization.where('users.id': current_user.id, id: params[:organization_id]).first

manu-d · 2017-11-15T10:11:57Z

api/lib/mno_enterprise/concerns/controllers/jpi/v1/admin/base_resource_controller.rb

  end

  def render_forbidden_request(attempted_action)
-    render json: { errors: {message: "Error while trying to #{attempted_action}: you do not have permission", code: 403} }, status: :forbidden
+    render json: { errors: { message: "Error while trying to #{attempted_action}: you do not have permission", code: 403 } }, status: :forbidden


Can't you put this 3 methods in application_controller.rb, so that you dont' have to repeat them as well in the non_admin base controller?

manu-d · 2017-11-15T10:15:54Z

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

@@ -18,5 +17,6 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::Impac::DashboardTemplatesC
  # GET /mnoe/jpi/v1/impac/dashboard_templates
  def index
    @templates = MnoEnterprise::Dashboard.published_templates.includes(*DASHBOARD_DEPENDENCIES)
+    @organizations = MnoEnterprise::Organization.where('user.ids': current_user.id)


@organizations = MnoEnterprise::Organization.where('users.id': current_user.id)

manu-d · 2017-11-15T10:17:35Z

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

@@ -18,11 +18,13 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::Impac::DashboardsControlle
  # GET /mnoe/jpi/v1/impac/dashboards
  def index
    dashboards
+    @organizations = MnoEnterprise::Organization.where('user.ids': current_user.id)


@organizations = MnoEnterprise::Organization.where('users.id': current_user.id)

manu-d · 2017-11-15T10:18:12Z

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

@@ -46,7 +49,7 @@ def update
    # TODO: enable authorization
    # authorize! :manage_dashboard, dashboard
    dashboard.update_attributes!(dashboard_update_params)
-
+    @organizations = MnoEnterprise::Organization.where('user.ids': current_user.id)


@organizations = MnoEnterprise::Organization.where('users.id': current_user.id)

manu-d · 2017-11-15T10:18:33Z

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

@@ -73,6 +76,7 @@ def copy
    # Owner is the current user by default, can be overriden to something else (eg: current organization)
    @dashboard = template.copy!(current_user, dashboard_params[:name], dashboard_params[:organization_ids])
    @dashboard = @dashboard.load_required(DASHBOARD_DEPENDENCIES)
+    @organizations = MnoEnterprise::Organization.where('user.ids': current_user.id)


@organizations = MnoEnterprise::Organization.where('users.id': current_user.id)

manu-d · 2017-11-15T10:23:57Z

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

@@ -17,7 +17,7 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::Impac::WidgetsController
  #  -> GET /api/mnoe/v1/organizations/:id/widgets
  def index
    render_not_found('organization') unless parent_organization
-    @widgets = MnoEnterprise::Widget.find(organization_id: parent_organization.id)
+    @widgets = MnoEnterprise::Widget.find(organization_id: parent_organization_id)


parent_organization_id can be an uid as well

@widgets = MnoEnterprise::Widget.where('organization.id': parent_organization.id)

manu-d · 2017-11-15T10:38:42Z

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


-    subscription = MnoEnterprise::Subscription.where(organization_id: parent_organization.id, id: params[:id]).first
+    subscription = MnoEnterprise::Subscription.where(organization_id: parent_organization_id, id: params[:id]).first


subscription = MnoEnterprise::Subscription.where('organization.id': parent_organization_id, id: params[:id]).first

manu-d · 2017-11-15T10:39:05Z

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


-    subscription = MnoEnterprise::Subscription.where(organization_id: parent_organization.id, id: params[:id]).first
+    subscription = MnoEnterprise::Subscription.where(organization_id: parent_organization_id, id: params[:id]).first


subscription = MnoEnterprise::Subscription.where('organization.id': parent_organization_id, id: params[:id]).first

ouranos

See comments.

Have you done some local performance testing?
I have the feeling that, in some cases, it might not be faster as you're trading a smaller response for more API requests.

ouranos · 2017-11-22T00:07:40Z

api/app/controllers/mno_enterprise/jpi/v1/admin/impac/dashboard_templates_controller.rb

@@ -1,6 +1,8 @@
 module MnoEnterprise
  class Jpi::V1::Admin::Impac::DashboardTemplatesController < Jpi::V1::Admin::BaseResourceController

+    before_action :load_organizations, except: [:destroy]


Where's @organizations used? Only in the view?
With the before_action you're loading organizations all the time, even if the find_one! or save! is going to fail

True. I'll update it.

ouranos · 2017-11-22T00:18:31Z

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

  end
+
+  def orga_relation
+    MnoEnterprise::OrgaRelation.where('user.id' => current_user.id, 'organization.id': params[:organization_id]).first


Do you need the full orga_relation here? Otherwise just add select(:id)?

Good point.

ouranos · 2017-11-22T00:25:05Z

core/lib/json_api_client_extension/has_one_extension.rb

+      class_eval <<-CODE
+        def #{attr_name}_id=(id)
+          # Uncomment when doing refactoring
+          # ActiveSupport::Deprecation.warn(self.class.name + ".#{attr_name}_id= Use relationships instead")


This should not be commented?

it should be not commented during development, but not for prod. I will add a check.

ouranos · 2017-11-22T00:26:04Z

core/lib/json_api_client_extension/json_api_client_orm_adapter.rb

@@ -15,14 +15,14 @@ def column_names

    # @see OrmAdapter::Base#get!
    def get!(id)
-      res = klass.includes(*klass::INCLUDED_DEPENDENCIES).find(wrap_key(id)).first
+      res = klass.find(wrap_key(id)).first


Are you removing the INCLUDED_DEPENDENCIES feature? Was it widely used or just for the current_user?

it was mainly used for the current_user, but also for other controllers. (for example when current_user.organizations was called)

ouranos · 2017-11-22T00:28:26Z

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

@@ -94,11 +94,6 @@ def add_user!(user, role = 'Member')
    MnoEnterprise::OrgaRelation.create!(organization_id: self.id, user_id: user.id, role: role)
  end

-  def provision_app_instance!(app_nid)


see #584
This is going to introduce regressions

x4d3 · 2017-11-22T08:51:04Z

I will check the performance. The problem is that includes does not really work in v2, as it does not paginates. On top of that the MnoHub calls can be very long to build up the user with all the dependencies. It's definitely more interesting to make more smaller calls, easy to cache and reproduce than complicated calls with multiple includes.

ouranos

LGTM

core/lib/mno_enterprise/concerns/controllers/auth/omniauth_callbacks_controller.rb still calls provision_app_instance!

(Might need an extra spec if this hasn't been picked up)

ouranos · 2017-11-30T06:43:04Z

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

@@ -59,4 +58,5 @@ def destroy
      head :bad_request
    end
  end
+


Empty line 👮‍♂️

alachaum

👍 looks good to me

…nship - Introduce HasOneExtension, generating getter and setter for has_one relations, making them saved in relationships, and allowing their retrieving from a relations cache - Remove useless timestamp method - Rplace is_integer by is_id that is more semantic - Try to not rely on parent_organization

Each controller must be in charge of managing its own payload.

- fix issue with provisioning + update spec - Add log warning in has_one in test and development

x4d3 added enhancement Status: In Progress labels Nov 1, 2017

x4d3 requested review from ouranos and alachaum November 1, 2017 17:56

ouranos reviewed Nov 2, 2017

View reviewed changes

x4d3 mentioned this pull request Nov 3, 2017

[MNO-695][MNOE-773] Api v2 Dashboard missing dependency for KPI Alert Recipients #535

Merged

x4d3 force-pushed the feature/add-relationship-helper branch from fef78ca to 6a09fe5 Compare November 8, 2017 16:54

ouranos added this to the v4.0 milestone Nov 9, 2017

x4d3 force-pushed the feature/add-relationship-helper branch from 455932a to 58cb46c Compare November 9, 2017 17:43

x4d3 changed the title ~~WIP [MNOE-688] Generate relation_id getter and setter for has_one relationship~~ WIP [MNOE-688] Generate getter and setter for has_one relationship Nov 9, 2017

x4d3 force-pushed the feature/add-relationship-helper branch from 58cb46c to 62459b1 Compare November 9, 2017 18:04

x4d3 mentioned this pull request Nov 10, 2017

Add authorization to Dashboard Controller and include owner #558

Open

x4d3 force-pushed the feature/add-relationship-helper branch 2 times, most recently from a702359 to 9715076 Compare November 14, 2017 16:07

x4d3 requested a review from manu-d November 14, 2017 16:14

manu-d suggested changes Nov 15, 2017

View reviewed changes

x4d3 changed the title ~~WIP [MNOE-688] Generate getter and setter for has_one relationship~~ [MNOE-688] Generate getter and setter for has_one relationship Nov 15, 2017

x4d3 added Status: Review Needed and removed Status: In Progress labels Nov 15, 2017

x4d3 assigned ouranos Nov 15, 2017

manu-d approved these changes Nov 21, 2017

View reviewed changes

ouranos reviewed Nov 22, 2017

View reviewed changes

x4d3 force-pushed the feature/add-relationship-helper branch 2 times, most recently from 7194ed3 to 0c97737 Compare November 27, 2017 17:30

ouranos reviewed Nov 30, 2017

View reviewed changes

ouranos mentioned this pull request Dec 1, 2017

[MNOE-146] Display teams admin panel #583

Merged

ouranos assigned alachaum and unassigned ouranos and alachaum Dec 1, 2017

ouranos assigned x4d3 Dec 1, 2017

x4d3 force-pushed the feature/add-relationship-helper branch from 7cad9eb to 5c456f3 Compare December 1, 2017 09:20

alachaum previously approved these changes Dec 4, 2017

View reviewed changes

x4d3 added 10 commits December 4, 2017 14:39

Improve performance by stopping loading everything in current_user

3810f74

Each controller must be in charge of managing its own payload.

Fix spec

d66cf3c

Fix 'user.ids' -> 'users.id'

96f265a

Add only needed fields for users

bfdb430

Moved common method to application_controller

05a08bf

Moved common method back to base_resource

a4b6982

Do not load_organizations in before_action

eadf81c

Apply code review comments

76edb00

- fix issue with provisioning + update spec - Add log warning in has_one in test and development

Fix and spec OmniauthCallbacksController.setup_apps

9a5b025

x4d3 force-pushed the feature/add-relationship-helper branch from 5c456f3 to 9a5b025 Compare December 4, 2017 14:41

x4d3 dismissed alachaum’s stale review via 9a5b025 June 12, 2023 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNOE-688] Generate getter and setter for has_one relationship #560

[MNOE-688] Generate getter and setter for has_one relationship #560

x4d3 commented Nov 1, 2017 •

edited

Loading

ouranos Nov 2, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

x4d3 Nov 15, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

manu-d Nov 15, 2017

ouranos left a comment

ouranos Nov 22, 2017

x4d3 Nov 22, 2017

ouranos Nov 22, 2017

x4d3 Nov 22, 2017

ouranos Nov 22, 2017

x4d3 Nov 22, 2017

ouranos Nov 22, 2017

x4d3 Nov 22, 2017

ouranos Nov 22, 2017

x4d3 commented Nov 22, 2017

ouranos left a comment

ouranos Nov 30, 2017

alachaum left a comment

[MNOE-688] Generate getter and setter for has_one relationship #560

Are you sure you want to change the base?

[MNOE-688] Generate getter and setter for has_one relationship #560

Conversation

x4d3 commented Nov 1, 2017 • edited Loading

has_one relationship helper

make current_user call smaller

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

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

x4d3 commented Nov 22, 2017

ouranos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alachaum left a comment

Choose a reason for hiding this comment

x4d3 commented Nov 1, 2017 •

edited

Loading