Skip to content
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-428] Migrate mno-enterprise to MnoHub API V2 #281

Merged
merged 5 commits into from
Jun 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class ImpersonateController < ApplicationController
# GET /impersonate/user/123
def create
session[:impersonator_redirect_path] = params[:redirect_path].presence
@user = MnoEnterprise::User.find(params[:user_id])
@user = MnoEnterprise::User.find_one(params[:user_id], :deletion_requests, :organizations, :orga_relations, :dashboards)
if @user.present?
impersonate(@user)
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ def create
# Invite for unconfirmed users are automatically accepted
def find_org_invite(organization, user)
if user.confirmed?
status_scope = { 'status.in' => %w(staged pending) }
status_scope = { status: %w(staged pending) }
else
status_scope = { status: 'accepted' }
end
organization.org_invites.where(status_scope.merge(user_id: user.id)).first
MnoEnterprise::OrgInvite.includes(:user).where(status_scope.merge(user_id: user.id, organization_id: organization.id)).first
end

# Send the org invite and update the status
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,15 @@ class Jpi::V1::AppInstancesSyncController < Jpi::V1::BaseResourceController
# GET /mnoe/jpi/v1/organization/org-fbba/app_instances_sync
def index
authorize! :check_apps_sync, @parent_organization
# find method is overriden in the mnoe interface to call organization.check_sync_apps_progress
connectors = @parent_organization.app_instances_sync.find('anything').connectors
connectors = parent_organization.app_instances_sync.first.connectors
render json: results(connectors)
end


# POST /mnoe/jpi/v1/organizations/org-fbba/app_instances_sync
def create
authorize! :sync_apps, @parent_organization

# Some weird behaviour with Her and has_one. If app_instances_sync.find is called somewhere before the create,
# Her won't detect the organization_id as dirty and won't submit it.
sync = @parent_organization.app_instances_sync.build(mode: params[:mode])
sync.organization_id_will_change!
sync.save

connectors = sync.connectors

connectors = parent_organization.trigger_app_instances_sync.first.connectors
render json: results(connectors)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,15 @@ class Jpi::V1::AuditEventsController < Jpi::V1::BaseResourceController

# GET /mnoe/jpi/v1/admin/audit_events
def index
@organization = MnoEnterprise::Organization.find(params.require(:organization_id))
@organization = MnoEnterprise::Organization.find_one(params.require(:organization_id))

authorize! :administrate, @organization

@audit_events = MnoEnterprise::AuditEvent.where(organization_id: @organization.id)
@audit_events = @audit_events.limit(params[:limit]) if params[:limit]
@audit_events = @audit_events.skip(params[:offset]) if params[:offset]
@audit_events = @audit_events.order_by(params[:order_by]) if params[:order_by]
@audit_events = @audit_events.where(params[:where]) if params[:where]
@audit_events = @audit_events.all.fetch

response.headers['X-Total-Count'] = @audit_events.metadata[:pagination][:count]
query = MnoEnterprise::AuditEvent.where(organization_id: @organization.id)
query = MnoEnterprise::AuditEvent.apply_query_params(query, params)

response.headers['X-Total-Count'] = query.meta.record_count
@audit_events = query.to_a
respond_to do |format|
format.json
format.csv do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,17 @@ def timestamp
@timestamp ||= (params[:timestamp] || 0).to_i
end

def is_integer?(string)
string.to_i.to_s == string
end

def parent_organization
@parent_organization ||= current_user.organizations.to_a.find do |o|
key = (params[:organization_id].to_i == 0) ? o.uid : o.id.to_s
key == params[:organization_id].to_s
@parent_organization ||= begin
id_or_uid = params[:organization_id]
query = is_integer?(id_or_uid) ? id_or_uid : {uid: id_or_uid}
o = MnoEnterprise::Organization.includes(:orga_relations, :users).find(query).first
## check that user is in the organization
o if o && o.orga_relation(current_user)
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,8 @@ json.stack app_instance.stack
json.name app_instance.name
json.status app_instance.status
json.oauth_keys_valid app_instance.oauth_keys_valid
#json.http_url app_instance.http_url
#json.microsoft_trial_url app_instance.microsoft_trial_url
json.created_at app_instance.created_at

json.per_user_licence app_instance.per_user_licence
json.licences_count app_instance.active_licences_count if app_instance.per_user_licence?

if app_instance.under_free_trial?
json.free_trial_end_at app_instance.free_trial_end_at
end

if app_instance.oauth_company
json.oauth_company_name app_instance.oauth_company
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
json.audit_events do
json.array! @audit_events, partial: 'audit_event', as: :audit_event
end
json.metadata @audit_events.metadata

Original file line number Diff line number Diff line change
Expand Up @@ -27,24 +27,26 @@ json.cache! ['v1', @user.cache_key] do
# Embed association if user is persisted
if @user.id
json.organizations do
json.array! (@user.organizations.active || []) do |o|
json.array! (@user.organizations.select(&:active?) || []) do |o|
json.id o.id
json.uid o.uid
json.name o.name
json.currency o.billing_currency
json.current_user_role o.role
json.has_myob_essentials_only o.has_myob_essentials_only?
json.current_user_role @user.role(o)
json.has_myob_essentials_only o.has_myob_essentials_only
json.financial_year_end_month o.financial_year_end_month
end
end

if @user.deletion_request.present?
if @user.current_deletion_request.present?
json.deletion_request do
json.extract! @user.deletion_request, :id, :token
json.extract! @user.current_deletion_request, :id, :token
end
end

json.kpi_enabled [email protected]_enabled
end
end
end


Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
json.ignore_nil!
json.extract! alert, :id, :title, :webhook, :service, :sent
json.metadata alert.settings
json.kpi_id alert.impac_kpi_id
json.kpi_id alert.kpi_id
json.recipients alert.recipients.map do |recipient|
json.extract! recipient, :id, :email
end
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,5 @@ end

json.kpis dashboard.kpis, partial: 'mno_enterprise/jpi/v1/impac/kpis/kpi', as: :kpi

json.widgets dashboard.widgets, partial: 'mno_enterprise/jpi/v1/impac/widgets/widget', as: :widget
json.widgets dashboard.sorted_widgets, partial: 'mno_enterprise/jpi/v1/impac/widgets/widget', as: :widget

json.widgets_templates dashboard.filtered_widgets_templates
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ json.name widget.name
json.endpoint (widget.endpoint || widget.widget_category)
json.width widget.width
json.metadata widget.settings
json.owner widget.owner
json.owner widget.dashboard_owner_uid

json.kpis widget.kpis, partial: 'mno_enterprise/jpi/v1/impac/kpis/kpi', as: :kpi
14 changes: 10 additions & 4 deletions api/app/views/mno_enterprise/jpi/v1/marketplace/_app.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@ json.average_rating app.average_rating
json.add_on app.add_on?
json.running_instances_count app.running_instances_count

json.shared_entities do
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response is changing from shared_entities to app_shared_entities?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

json.array! app.app_shared_entities do |shared_entity|
json.shared_entity_nid shared_entity.shared_entity.nid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

json.shared_entity_name shared_entity.shared_entity&.name
json.shared_entity_name shared_entity.shared_entity&.name
json.write shared_entity.write
json.read shared_entity.read
end
end
end if app.shared_entities.any?
end

if app.logo
json.logo app.logo.to_s
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
json.invoices do
# TODO: Introduce pagination
json.array! organization.invoices.order_by('ended_at.desc').limit(12) do |invoice|
json.period invoice.period_label
json.amount AccountingjsSerializer.serialize(invoice.total_due)
json.paid invoice.paid?
json.link mno_enterprise.invoice_path(invoice.slug)
end
json.array! []
# TODO [MIGRATION_V2] remove and replace by a call to /organizations/id/invoices

end
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ if member.is_a?(MnoEnterprise::User)
json.name member.name
json.surname member.surname
json.email member.email
json.role member.role
elsif member.is_a?(MnoEnterprise::OrgInvite)
json.role organization.role(member)
elsif member.is_a?(MnoEnterprise::OrgaInvite)
json.id member.id
json.entity 'OrgInvite'
json.email member.user_email
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
org = @parent_organization || team.organization
@all_apps ||= MnoEnterprise::App.all.to_a

json.id team.id
Expand All @@ -7,10 +6,7 @@ json.name team.name
json.users do
json.array! team.users do |user|
json.extract! user, :id, :name, :surname, :email

# Retrieve role from cached version (org user list)
org_user = org.users.to_a.find { |e| e.id == user.id }
json.role org_user ? org_user.role : nil
json.role @parent_organization.role(user)
end
end

Expand Down
2 changes: 1 addition & 1 deletion api/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
end
end
# Maestrano-hub events
resources :events, only: [:create]
resources :events, only: [:create]
end

#============================================================
Expand Down
28 changes: 10 additions & 18 deletions api/lib/mno_enterprise/audit_events_listener.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

basic_auth MnoEnterprise.tenant_id, MnoEnterprise.tenant_key

def info(key, current_user_id, description, subject_type, subject_id, metadata)
data = {
key: key,
user_id: current_user_id,
description: description,
metadata: metadata,
subject_type: subject_type,
subject_id: subject_id,
}
organization_id = if (subject_type == 'MnoEnterprise::Organization') then
subject_id
elsif metadata.is_a?(Hash)
metadata[:organization_id].presence
end
body = {
data: {
key: key,
user_id: current_user_id,
description: description,
metadata: metadata,
subject_type: subject_type,
subject_id: subject_id,
}
}
body[:data][:organization_id] = organization_id if organization_id
self.class.post('', body: body)
rescue Net::ReadTimeout
# Meant to fail
data[:organization_id] = organization_id if organization_id
MnoEnterprise::AuditEvent.create(data)
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ module MnoEnterprise::Concerns::Controllers::DeletionRequestsController
before_filter :set_meta

def set_meta
@meta[:title] = "Account Termination"
@meta[:description] = "Account Termination"
@meta[:title] = 'Account Termination'
@meta[:description] = 'Account Termination'
end
end

Expand All @@ -33,7 +33,7 @@ module ClassMethods
# GET /deletion_requests/1
def show
# authorize! :manage_billing, current_user.organizations.find(@invoice.organization_id)
@deletion_request = current_user.deletion_request
@deletion_request = current_user.current_deletion_request

respond_to do |format|
# Check that the user has a deletion_request in progress
Expand All @@ -58,7 +58,7 @@ def show

# PATCH /deletion_requests/1/freeze_account
def freeze_account
@deletion_request = current_user.deletion_request
@deletion_request = current_user.current_deletion_request

respond_to do |format|
# Check that the user has a deletion_request in progress
Expand All @@ -81,7 +81,7 @@ def freeze_account

# PATCH /deletion_requests/1/checkout
def checkout
@deletion_request = current_user.deletion_request
@deletion_request = current_user.current_deletion_request

respond_to do |format|
# Check that the user has a deletion_request in progress
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,31 +13,33 @@ 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/app_instances
def index
@app_instances = parent_organization.app_instances.active.where("updated_at.gt" => Time.at(timestamp)).select do |i|
# force owner assignment to avoid a refetch in ability can?(:access,i)
i.owner = parent_organization
@app_instances = MnoEnterprise::AppInstance.includes(:app, :owner).where(owner_id: parent_organization.id, 'status.in': MnoEnterprise::AppInstance::ACTIVE_STATUSES).to_a.select do |i|
can?(:access,i)
end
end

# POST /mnoe/jpi/v1/organization/1/app_instances
def create
authorize! :manage_app_instances, parent_organization
app_instance = parent_organization.app_instances.create(product: params[:nid])
MnoEnterprise::EventLogger.info('app_add', current_user.id, 'App added', app_instance)
head :created
app_instance = parent_organization.provision_app_instance(params[:nid])
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@x4d3 x4d3 Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

head :created
end
end

# DELETE /mnoe/jpi/v1/app_instances/1
def destroy
app_instance = MnoEnterprise::AppInstance.find(params[:id])
@app_instance = MnoEnterprise::AppInstance.find_one(params[:id])

if app_instance
authorize! :manage_app_instances, app_instance.owner
MnoEnterprise::EventLogger.info('app_destroy', current_user.id, 'App destroyed', app_instance)
app_instance.terminate
if @app_instance
authorize! :manage_app_instances, @app_instance.owner
MnoEnterprise::EventLogger.info('app_destroy', current_user.id, 'App destroyed', @app_instance)
@app_instance = @app_instance.terminate.first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here first is counter-intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know,

JsonApiClient/json_api_client#75

This was intentional for a few reasons:

First, I always wanted to make a result set be available because there is extra meta data that comes back with the response. We could embed the meta data in a single resource object but that seems weird because it wouldn't be available on the resource objects fetched in a multi object response. I guess we could store a reference to the meta data but that also seems a bit janky.

Second, in the initial version of jsonapi, all responses returned arrays - even requests for a single resource. The rationale behind that was to make client side parsing easier - no matter what you were fetching, you could always iterate of the results like an array. There was a change (about a year ago?) to the spec that brought back the ability to respond with a single object or and array of objects as the main data. I wasn't a huge fan of it at the time.

Additionally, I really don't like the behavior in ActiveRecord where find can return a single result or an array depending on what input you give it. This departs from ActiveRecord but I'm ok with that here.

Hope this gives some insight into why it's doing this.

end

head :accepted
Expand Down
Loading