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

Move meta related actions from ProjectController to its own CRUD controller #6422

Merged
merged 2 commits into from Dec 5, 2018
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 3 additions & 48 deletions src/api/app/controllers/webui/project_controller.rb
Expand Up @@ -14,15 +14,15 @@ class Webui::ProjectController < Webui::WebuiController
:new_package, :new_package_branch, :incident_request_dialog, :release_request_dialog,
:show, :linking_projects, :add_person, :add_group, :buildresult, :delete_dialog,
:destroy, :remove_path_from_target, :rebuild_time, :packages_simple,
:requests, :save, :monitor, :toggle_watch, :meta,
:requests, :save, :monitor, :toggle_watch,
vpereira marked this conversation as resolved.
Show resolved Hide resolved
:prjconf, :edit, :edit_comment,
:status, :maintained_projects,
:add_maintained_project_dialog, :add_maintained_project, :remove_maintained_project,
:maintenance_incidents, :unlock_dialog, :unlock, :save_person, :save_group, :remove_role,
:move_path, :save_prjconf, :clear_failed_comment, :pulse]

# TODO: check if get_by_name or set_by_name is used for save_prjconf
before_action :set_project_by_name, only: [:save_meta, :save_prjconf]
before_action :set_project_by_name, only: [:save_prjconf]

before_action :set_project_by_id, only: [:update]

Expand All @@ -39,7 +39,7 @@ class Webui::ProjectController < Webui::WebuiController

before_action :set_maintained_project, only: [:remove_maintained_project]

after_action :verify_authorized, only: [:save_new, :new_incident, :save_meta]
after_action :verify_authorized, only: [:save_new, :new_incident]

def index
@show_all = (params[:show_all].to_s == 'true')
Expand Down Expand Up @@ -503,51 +503,6 @@ def toggle_watch
end
end

def meta
@meta = @project.render_xml
switch_to_webui2
end

def save_meta
authorize @project, :update?

errors = []
begin
Suse::Validator.validate('project', params[:meta])
request_data = Xmlhash.parse(params[:meta])

remove_repositories = @project.get_removed_repositories(request_data)
errors << Project.check_repositories(remove_repositories)[:error]
errors << Project.validate_remote_permissions(request_data)[:error]
errors << Project.validate_link_xml_attribute(request_data, @project.name)[:error]
errors << Project.validate_maintenance_xml_attribute(request_data)[:error]
errors << Project.validate_repository_xml_attribute(request_data, @project.name)[:error]

errors = errors.compact

if errors.empty?
Project.transaction do
errors << @project.update_from_xml(request_data)[:error]
errors = errors.compact
@project.store if errors.empty?
end
end
rescue Suse::ValidationError => exception
errors << exception.message
end

status = if errors.empty?
flash.now[:success] = 'Config successfully saved!'
200
else
flash.now[:error] = errors.compact.join("\n")
400
end
switch_to_webui2
namespace = switch_to_webui2? ? 'webui2' : 'webui'
render layout: false, status: status, partial: "layouts/#{namespace}/flash", object: flash
end

def prjconf
sliced_params = params.slice(:rev)
sliced_params.permit!
Expand Down
47 changes: 47 additions & 0 deletions src/api/app/controllers/webui/projects/meta_controller.rb
@@ -0,0 +1,47 @@
module Webui
module Projects
class MetaController < WebuiController
before_action :set_project
before_action :validate_meta, only: [:update], if: -> { params[:meta] }
after_action :verify_authorized, only: [:update]

def show
@meta = @project.render_xml
switch_to_webui2
end

def update
authorize @project, :update?
updater = ::MetaControllerService::ProjectUpdater.new(project: @project, request_data: @request_data)
updater.call

status = if updater.valid?
flash.now[:success] = 'Config successfully saved!'
200
else
flash.now[:error] = updater.errors
400
end
switch_to_webui2
render layout: false, status: status, partial: "layouts/#{view_namespace}/flash", object: flash
end

private

def validate_meta
meta_validator = ::MetaControllerService::MetaXMLValidator.new(params)
meta_validator.call
if meta_validator.errors?
flash.now[:error] = meta_validator.errors
render layout: false, status: 400, partial: "layouts/#{view_namespace}/flash", object: flash
else
@request_data = meta_validator.request_data
end
end

def view_namespace
switch_to_webui2? ? 'webui2' : 'webui'
end
end
end
end
25 changes: 25 additions & 0 deletions src/api/app/services/meta_controller_service/meta_validator.rb
@@ -0,0 +1,25 @@
module MetaControllerService
class MetaValidator
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the class name reflects what this class is doing, it does not validate the Meta but the XML, so MetaXmlValidator might be a better name.

attr_reader :project, :request_data, :errors

def initialize(params = {})
@project = params[:project]
@request_data = params[:request_data]
@errors = []
end

def call
remove_repositories = @project.get_removed_repositories(@request_data)
@errors << Project.check_repositories(remove_repositories)[:error]
@errors << Project.validate_remote_permissions(@request_data)[:error]
@errors << Project.validate_link_xml_attribute(@request_data, @project.name)[:error]
@errors << Project.validate_maintenance_xml_attribute(@request_data)[:error]
@errors << Project.validate_repository_xml_attribute(@request_data, @project.name)[:error]
@errors.compact!
end

def valid?
@errors.empty?
end
end
end
22 changes: 22 additions & 0 deletions src/api/app/services/meta_controller_service/meta_xml_validator.rb
@@ -0,0 +1,22 @@
module MetaControllerService
class MetaXMLValidator
require_dependency 'opensuse/validator'

attr_reader :meta, :request_data, :errors

def initialize(params = {})
@meta = params[:meta]
end

def call
Suse::Validator.validate('project', @meta)
@request_data = Xmlhash.parse(@meta)
rescue Suse::ValidationError => exception
@errors = exception.message
end

def errors?
@errors.present?
end
end
end
30 changes: 30 additions & 0 deletions src/api/app/services/meta_controller_service/project_updater.rb
@@ -0,0 +1,30 @@
module MetaControllerService
class ProjectUpdater
def initialize(project: nil, request_data: {}, validator_klass: ::MetaControllerService::MetaValidator)
@project = project
@request_data = request_data
@validator = validator_klass.new(project: project, request_data: request_data)
end

def call
Copy link
Member

Choose a reason for hiding this comment

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

[nitpick] I would prefer to use save here to keep same interface as ActiveRecord.

@validator.call
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to get rid of this additional call call. I think this should behave similar to ActiveRecord which also does not require an additional method call before calling valid?. See my other comment.

unless @validator.valid?
@errors = @validator.errors
return
end

Project.transaction do
@errors = @project.update_from_xml(@request_data)[:error]
@project.store if valid?
end
end

def errors
@errors.is_a?(Array) ? @errors.join("\n") : @errors
Copy link
Member

Choose a reason for hiding this comment

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

Why not always having an array? Just instantiate the error instance variable with an empty array and use << instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can hide this implementation detail out of the controller. So if its ok I would keep it where it is.

end

def valid?
@validator.valid? && @errors.blank?
end
end
end
4 changes: 2 additions & 2 deletions src/api/app/views/webui/project/_tabs.html.erb
Expand Up @@ -22,7 +22,7 @@
<%= tab 'users', 'Users', :controller => '/webui/project', :action => :users unless @project.defines_remote_instance? %>
<%= tab 'subprojects', 'Subprojects', :controller => '/webui/project', :action => :subprojects unless @project.defines_remote_instance? || @is_maintenance_project %>
<% end -%>

<% if is_advanced_tab? %>
<%= content_for :ready_function do %>
$("#advanced_tabs").show();
Expand All @@ -36,7 +36,7 @@
<%= tab 'projectconfig', 'Project Config', :controller => '/webui/project', :action => :prjconf unless @project.defines_remote_instance? || @is_maintenance_project %>
<% unless @spider_bot -%>
<%= tab 'attribute', 'Attributes', :controller => '/webui/attribute', :project => @project, :action => 'index' %>
<%= tab 'meta', "Meta", :controller => '/webui/project', :action => :meta %>
<%= tab 'meta', "Meta", :controller => '/webui/projects/meta', :action => :show %>
<%= tab 'status', 'Status', :controller => '/webui/project', :action => :status unless @project.defines_remote_instance? || @is_maintenance_project %>
<%= tab 'pulse', 'Pulse', :controller => '/webui/project', :action => :pulse %>
<% end -%>
Expand Down
2 changes: 1 addition & 1 deletion src/api/app/views/webui/project/show.html.erb
Expand Up @@ -71,7 +71,7 @@
<% if @releasetargets.length > 0 %>
<li>
<%= sprite_tag 'information' %>
<%= link_to pluralize(@releasetargets.length, "Release Target"), action: 'meta', project: @project %>
<%= link_to pluralize(@releasetargets.length, "Release Target"), project_meta_path(project: @project) %>
</li>
<% end %>
<% end %>
Expand Down
Expand Up @@ -4,11 +4,11 @@
<% @metarobots = 'noindex' %>
<% project_bread_crumb('Meta') %>
<%= render :partial => 'tabs' %>
<%= render :partial => 'webui/project/tabs' %>

<h3><%= @pagetitle %></h3>
<% if User.current.can_modify?(@project) %>
<%= render partial: "shared/editor", locals: {text: @meta, mode: 'xml', save: {url: url_for(controller: 'project', action: 'save_meta'), method: 'POST', data: {project: @project.name, submit: 'meta'}}} %>
<%= render partial: "shared/editor", locals: {text: @meta, mode: 'xml', save: {url: project_save_meta_path(@project), method: 'POST', data: {project: @project.name, submit: 'meta'}}} %>
<% else %>
<%= render partial: "shared/editor", locals: {text: @meta, mode: 'xml', style: {read_only: true}} %>
<% end %>
Expand Up @@ -19,6 +19,3 @@
- elsif current_page?(project_config_path(@project))
%li.breadcrumb-item.active{ 'aria-current' => 'page' }
Configuration
- elsif current_page?(project_meta_path(@project))
%li.breadcrumb-item.active{ 'aria-current' => 'page' }
Meta
@@ -0,0 +1,3 @@
= render partial: 'webui/project/breadcrumb_items'
dmarcoux marked this conversation as resolved.
Show resolved Hide resolved
%li.breadcrumb-item.active{ 'aria-current' => 'page' }
Meta
7 changes: 5 additions & 2 deletions src/api/config/routes.rb
Expand Up @@ -231,6 +231,11 @@ def self.public_or_about_path?(request)
end
end

controller 'webui/projects/meta' do
get 'project/meta/:project' => :show, constraints: cons, as: 'project_meta'
post 'project/save_meta/:project' => :update, constraints: cons, as: :project_save_meta
end

controller 'webui/project' do
get 'project/' => :index, as: 'projects'
get 'project/list_public' => :index
Expand Down Expand Up @@ -280,8 +285,6 @@ def self.public_or_about_path?(request)
get 'project/package_buildresult/:project' => :package_buildresult, constraints: cons
# TODO: this should be POST (and the link AJAX)
get 'project/toggle_watch/:project' => :toggle_watch, constraints: cons, as: 'project_toggle_watch'
get 'project/meta/:project' => :meta, constraints: cons, as: 'project_meta'
post 'project/save_meta/:project' => :save_meta, constraints: cons, as: :project_save_meta
get 'project/prjconf/:project' => :prjconf, constraints: cons, as: :project_config
post 'project/save_prjconf/:project' => :save_prjconf, constraints: cons, as: :save_project_config
get 'project/clear_failed_comment/:project' => :clear_failed_comment, constraints: cons
Expand Down