-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MNOE-978] Refactor pagination #673
base: 4.0
Are you sure you want to change the base?
Conversation
@@ -1,2 +1,2 @@ | |||
json.extract! organization, :id, :name, :uid, :soa_enabled, :created_at, :account_frozen, :financial_metrics, :billing_currency, :external_id, :belong_to_sub_tenant, :belong_to_account_manager, :demo_account | |||
|
|||
json.role @user.role(organization) if @user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since @user
is being passed into the partial in the view (...admin/organizations/show.json.jbuilder), can it be referred to as just user
here or can the user: @user
be removed from the show view?
@@ -26,6 +26,10 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::Admin::OrganizationsContro | |||
#================================================================== | |||
# GET /mnoe/jpi/v1/admin/organizations | |||
def index | |||
# When fetching multiple organizations attached to one user, we must fetch the user's role for each organization. | |||
where = params["where"] | |||
@user = MnoEnterprise::User.find_one(params["where"]["users.id"], :orga_relations) if where && where["users.id"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we refactor this like this maybe?
@user = MnoEnterprise::User.find_one(params["where"]["users.id"], :orga_relations) if params.dig('where', 'users.id')
Then the assignment on line 30 can be removed.
ffda3dc
to
bc501ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
bc501ad
to
16aadca
Compare
No description provided.