From ed60751dfac549fc3621c337db711e0a05ad2018 Mon Sep 17 00:00:00 2001 From: Pavel Chipiga Date: Tue, 21 Feb 2012 01:13:05 +0200 Subject: [PATCH] Delegate all repo abilities to platform. Remove repository owner and relations. Fix templates and specs. Refactor and code cleanup. Refs #205 --- .../personal_repositories_controller.rb | 2 +- app/controllers/repositories_controller.rb | 1 - app/models/ability.rb | 14 +++--- app/models/group.rb | 21 ++++---- app/models/relation.rb | 7 +-- app/models/repository.rb | 50 ++++++------------- app/models/user.rb | 15 +++--- .../projects_list.html.haml | 10 ---- .../personal_repositories/show.html.haml | 6 --- .../repositories/projects_list.html.haml | 5 -- app/views/repositories/show.html.haml | 5 -- ...0120220185458_remove_repositories_owner.rb | 12 +++++ db/schema.rb | 4 +- lib/modules/models/personal_repository.rb | 1 - .../collaborators_controller_spec.rb | 1 + spec/controllers/groups_controller_spec.rb | 1 + spec/controllers/members_controller_spec.rb | 1 + .../personal_repositories_controller_spec.rb | 5 +- .../repositories_controller_spec.rb | 6 +-- spec/factories/repository_factory.rb | 16 ++---- spec/models/cancan_spec.rb | 11 ++-- spec/models/comment_for_commit_spec.rb | 1 + spec/models/comment_spec.rb | 1 + spec/models/repository_spec.rb | 11 ++-- spec/models/subscribe_spec.rb | 1 + 25 files changed, 77 insertions(+), 131 deletions(-) create mode 100644 db/migrate/20120220185458_remove_repositories_owner.rb diff --git a/app/controllers/personal_repositories_controller.rb b/app/controllers/personal_repositories_controller.rb index c76c21623..122ed928a 100644 --- a/app/controllers/personal_repositories_controller.rb +++ b/app/controllers/personal_repositories_controller.rb @@ -12,7 +12,7 @@ class PersonalRepositoriesController < ApplicationController else @projects = @repository.projects.recent.paginate :page => params[:project_page], :per_page => 30 end - @user = @repository.owner + @user = @repository.platform.owner @urpmi_commands = @repository.platform.urpmi_list(request.host) end diff --git a/app/controllers/repositories_controller.rb b/app/controllers/repositories_controller.rb index 317683699..693be6034 100644 --- a/app/controllers/repositories_controller.rb +++ b/app/controllers/repositories_controller.rb @@ -42,7 +42,6 @@ class RepositoriesController < ApplicationController def create @repository = Repository.new(params[:repository]) @repository.platform_id = params[:platform_id] - @repository.owner = get_owner if @repository.save flash[:notice] = t('flash.repository.saved') redirect_to @repositories_path diff --git a/app/models/ability.rb b/app/models/ability.rb index 2b58c1329..8a6ac63a4 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -66,18 +66,16 @@ class Ability can :read, Platform, :owner_type => 'User', :owner_id => user.id can :read, Platform, :owner_type => 'Group', :owner_id => user.group_ids can(:read, Platform, read_relations_for('platforms')) {|platform| local_reader? platform} - can(:update, Platform) {|platform| local_admin? platform} + can([:update, :build_all], Platform) {|platform| local_admin? platform} can([:freeze, :unfreeze, :destroy], Platform) {|platform| owner? platform} can :autocomplete_user_uname, Platform - # TODO delegate to platform? can :read, Repository, :platform => {:visibility => 'open'} - can :read, Repository, :owner_type => 'User', :owner_id => user.id - can :read, Repository, :owner_type => 'Group', :owner_id => user.group_ids - can(:read, Repository, read_relations_for('repositories')) {|repository| local_reader? repository} - can(:create, Repository) {|repository| local_admin? repository.platform} - can([:update, :add_project, :remove_project], Repository) {|repository| local_admin? repository} - can([:change_visibility, :settings, :destroy], Repository) {|repository| owner? repository} + can :read, Repository, :platform => {:owner_type => 'User', :owner_id => user.id} + can :read, Repository, :platform => {:owner_type => 'Group', :owner_id => user.group_ids} + can(:read, Repository, read_relations_for('repositories', 'platforms')) {|repository| local_reader? repository.platform} + can([:create, :update, :projects_list, :add_project, :remove_project], Repository) {|repository| local_admin? repository.platform} + can([:change_visibility, :settings, :destroy], Repository) {|repository| owner? repository.platform} can :read, Product, :platform => {:owner_type => 'User', :owner_id => user.id} can :read, Product, :platform => {:owner_type => 'Group', :owner_id => user.group_ids} diff --git a/app/models/group.rb b/app/models/group.rb index c68147305..a7d96dcc7 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -2,16 +2,16 @@ class Group < ActiveRecord::Base belongs_to :owner, :class_name => 'User' - has_many :own_projects, :as => :owner, :class_name => 'Project' - + has_many :relations, :as => :object, :dependent => :destroy has_many :objects, :as => :target, :class_name => 'Relation' has_many :targets, :as => :object, :class_name => 'Relation' has_many :members, :through => :objects, :source => :object, :source_type => 'User', :autosave => true has_many :projects, :through => :targets, :source => :target, :source_type => 'Project', :autosave => true - has_many :platforms, :through => :targets, :source => :target, :source_type => 'Platform', :autosave => true, :dependent => :destroy - has_many :repositories, :through => :targets, :source => :target, :source_type => 'Repository', :autosave => true - has_many :relations, :as => :object, :dependent => :destroy + has_many :platforms, :through => :targets, :source => :target, :source_type => 'Platform', :autosave => true + + has_many :own_projects, :as => :owner, :class_name => 'Project', :dependent => :destroy + has_many :own_platforms, :as => :owner, :class_name => 'Platform', :dependent => :destroy validates :name, :owner, :presence => true validates :uname, :presence => true, :uniqueness => {:case_sensitive => false}, :format => { :with => /^[a-z0-9_]+$/ } @@ -26,11 +26,12 @@ class Group < ActiveRecord::Base after_initialize lambda {|r| r.name ||= r.uname } # default include Modules::Models::PersonalRepository -# include Modules::Models::Owner + # include Modules::Models::Owner protected - def add_owner_to_members - Relation.create_with_role(self.owner, self, 'admin') -# members << self.owner if !members.exists?(:id => self.owner.id) - end + + def add_owner_to_members + Relation.create_with_role(self.owner, self, 'admin') + # members << self.owner if !members.exists?(:id => self.owner.id) + end end diff --git a/app/models/relation.rb b/app/models/relation.rb index 59c31d29d..6847b097b 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -22,7 +22,8 @@ class Relation < ActiveRecord::Base end protected - def add_default_role - self.role = ROLES.first if role.nil? || role.empty? - end + + def add_default_role + self.role = ROLES.first if role.nil? || role.empty? + end end diff --git a/app/models/repository.rb b/app/models/repository.rb index 379d3deac..f5c7d4c13 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1,27 +1,19 @@ # -*- encoding : utf-8 -*- class Repository < ActiveRecord::Base belongs_to :platform - belongs_to :owner, :polymorphic => true has_many :projects, :through => :project_to_repositories #, :dependent => :destroy has_many :project_to_repositories, :validate => true, :dependent => :destroy - has_many :relations, :as => :target, :dependent => :destroy - has_many :objects, :as => :target, :class_name => 'Relation', :dependent => :destroy - has_many :members, :through => :objects, :source => :object, :source_type => 'User' - has_many :groups, :through => :objects, :source => :object, :source_type => 'Group' - validates :description, :uniqueness => {:scope => :platform_id}, :presence => true validates :name, :uniqueness => {:scope => :platform_id, :case_sensitive => false}, :presence => true, :format => { :with => /^[a-z0-9_\-]+$/ } - # validates :platform_id, :presence => true # if you uncomment this platform clone will not work scope :recent, order("name ASC") before_create :xml_rpc_create, :unless => lambda {Thread.current[:skip]} before_destroy :xml_rpc_destroy - after_create :add_admin_relations - attr_accessible :description, :name #, :platform_id + attr_accessible :description, :name def full_clone(attrs) # owner clone.tap do |c| # dup @@ -31,8 +23,6 @@ class Repository < ActiveRecord::Base end end - include Modules::Models::Owner - class << self def build_stub(platform) rep = Repository.new @@ -43,31 +33,21 @@ class Repository < ActiveRecord::Base protected - def xml_rpc_create - result = BuildServer.create_repo name, platform.name - if result == BuildServer::SUCCESS - return true - else - raise "Failed to create repository #{name} inside platform #{platform.name} with code #{result}." - end + def xml_rpc_create + result = BuildServer.create_repo name, platform.name + if result == BuildServer::SUCCESS + return true + else + raise "Failed to create repository #{name} inside platform #{platform.name} with code #{result}." end + end - def xml_rpc_destroy - result = BuildServer.delete_repo name, platform.name - if result == BuildServer::SUCCESS - return true - else - raise "Failed to delete repository #{name} inside platform #{platform.name} with code #{result}." - end - end - - def add_admin_relations - platform.relations.where(:role => 'admin').each do |rel| - if !relations.exists?(:role => 'admin', :object_type => rel.object_type, :object_id => rel.object_id) && rel.object != owner - r = relations.build(:role => 'admin', :object_type => rel.object_type) - r.object_id = rel.object_id - r.save - end - end + def xml_rpc_destroy + result = BuildServer.delete_repo name, platform.name + if result == BuildServer::SUCCESS + return true + else + raise "Failed to delete repository #{name} inside platform #{platform.name} with code #{result}." end + end end diff --git a/app/models/user.rb b/app/models/user.rb index 69dac7a6a..51eaba5b4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -11,22 +11,19 @@ class User < ActiveRecord::Base has_many :authentications, :dependent => :destroy has_many :build_lists, :dependent => :destroy + has_many :subscribes, :foreign_key => :user_id, :dependent => :destroy + has_many :comments, :dependent => :destroy has_many :relations, :as => :object, :dependent => :destroy has_many :targets, :as => :object, :class_name => 'Relation' - has_many :own_projects, :as => :owner, :class_name => 'Project', :dependent => :destroy - has_many :own_groups, :foreign_key => :owner_id, :class_name => 'Group' - has_many :own_platforms, :as => :owner, :class_name => 'Platform', :dependent => :destroy - has_many :own_repositories, :as => :owner, :class_name => 'Repository', :dependent => :destroy - - has_many :groups, :through => :targets, :source => :target, :source_type => 'Group', :autosave => true has_many :projects, :through => :targets, :source => :target, :source_type => 'Project', :autosave => true + has_many :groups, :through => :targets, :source => :target, :source_type => 'Group', :autosave => true has_many :platforms, :through => :targets, :source => :target, :source_type => 'Platform', :autosave => true - has_many :repositories, :through => :targets, :source => :target, :source_type => 'Repository', :autosave => true - has_many :subscribes, :foreign_key => :user_id, :dependent => :destroy - has_many :comments, :dependent => :destroy + has_many :own_projects, :as => :owner, :class_name => 'Project', :dependent => :destroy + has_many :own_groups, :foreign_key => :owner_id, :class_name => 'Group', :dependent => :destroy + has_many :own_platforms, :as => :owner, :class_name => 'Platform', :dependent => :destroy include Modules::Models::PersonalRepository diff --git a/app/views/personal_repositories/projects_list.html.haml b/app/views/personal_repositories/projects_list.html.haml index 4766fcb0b..2f3d8c3c0 100644 --- a/app/views/personal_repositories/projects_list.html.haml +++ b/app/views/personal_repositories/projects_list.html.haml @@ -20,21 +20,11 @@ = t("activerecord.attributes.repository.platform") \: = link_to @repository.platform.name, url_for(@repository.platform) - %p - %b - = t("activerecord.attributes.repository.owner") - \: - = link_to @repository.owner.name, url_for(@repository.owner) %p %b = t("activerecord.attributes.platform.visibility") \: = @repository.platform.visibility - %p - %b - = t("activerecord.attributes.repository.platform") - \: - = link_to @repository.platform.name, platform_path(@platform) .wat-cf = link_to image_tag("web-app-theme/icons/cross.png", :alt => t("layout.delete")) + " " + t("layout.delete"), @repository_path, :method => "delete", :class => "button", :confirm => t("layout.repositories.confirm_delete") diff --git a/app/views/personal_repositories/show.html.haml b/app/views/personal_repositories/show.html.haml index f8f346094..c40d3a1d5 100644 --- a/app/views/personal_repositories/show.html.haml +++ b/app/views/personal_repositories/show.html.haml @@ -6,12 +6,6 @@ %li= link_to t("layout.personal_repositories.private_users"), platform_private_users_path(@repository.platform) if @repository.platform.hidden? .content .inner - %p - %b - = t("activerecord.attributes.repository.owner") - \: - = link_to @repository.owner.name, url_for(@repository.owner) - = render 'shared/urpmi_list', :urpmi_commands => @urpmi_commands .wat-cf =# link_to image_tag("web-app-theme/icons/cross.png", :alt => t("layout.delete")) + " " + t("layout.delete"), @repository_path, :method => "delete", :class => "button", :confirm => t("layout.repositories.confirm_delete") diff --git a/app/views/repositories/projects_list.html.haml b/app/views/repositories/projects_list.html.haml index af5a9a1a9..d9ca336d8 100644 --- a/app/views/repositories/projects_list.html.haml +++ b/app/views/repositories/projects_list.html.haml @@ -21,11 +21,6 @@ = t("activerecord.attributes.repository.platform") \: = link_to @repository.platform.description, url_for(@repository.platform) - %p - %b - = t("activerecord.attributes.repository.owner") - \: - = link_to @repository.owner.uname, url_for(@repository.owner) .wat-cf = link_to image_tag("web-app-theme/icons/cross.png", :alt => t("layout.delete")) + " " + t("layout.delete"), @repository_path, :method => "delete", :class => "button", :confirm => t("layout.repositories.confirm_delete") diff --git a/app/views/repositories/show.html.haml b/app/views/repositories/show.html.haml index 72ab6577a..7e23d2b0d 100644 --- a/app/views/repositories/show.html.haml +++ b/app/views/repositories/show.html.haml @@ -21,11 +21,6 @@ = t("activerecord.attributes.repository.platform") \: = link_to @repository.platform.description, url_for(@repository.platform) - %p - %b - = t("activerecord.attributes.repository.owner") - \: - = link_to @repository.owner.try(:name), url_for(@repository.owner) .wat-cf = link_to image_tag("web-app-theme/icons/cross.png", :alt => t("layout.delete")) + " " + t("layout.delete"), @repository_path, :method => "delete", :class => "button", :confirm => t("layout.repositories.confirm_delete") if can? :destroy, @repository diff --git a/db/migrate/20120220185458_remove_repositories_owner.rb b/db/migrate/20120220185458_remove_repositories_owner.rb new file mode 100644 index 000000000..88c85a855 --- /dev/null +++ b/db/migrate/20120220185458_remove_repositories_owner.rb @@ -0,0 +1,12 @@ +class RemoveRepositoriesOwner < ActiveRecord::Migration + def self.up + remove_column :repositories, :owner_id + remove_column :repositories, :owner_type + Relation.delete_all(:target_type => 'Repository') + end + + def self.down + add_column :repositories, :owner_id, :integer + add_column :repositories, :owner_type, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 2498d7f31..6814bbbc7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20120210141153) do +ActiveRecord::Schema.define(:version => 20120220185458) do create_table "arches", :force => true do |t| t.string "name", :null => false @@ -294,8 +294,6 @@ ActiveRecord::Schema.define(:version => 20120210141153) do t.datetime "created_at" t.datetime "updated_at" t.string "name", :null => false - t.integer "owner_id" - t.string "owner_type" end create_table "rpms", :force => true do |t| diff --git a/lib/modules/models/personal_repository.rb b/lib/modules/models/personal_repository.rb index 58f8f1805..5b903ecae 100644 --- a/lib/modules/models/personal_repository.rb +++ b/lib/modules/models/personal_repository.rb @@ -20,7 +20,6 @@ module Modules pl.save! rep = pl.repositories.build - rep.owner = pl.owner rep.name = 'main' rep.description = 'main' rep.save! diff --git a/spec/controllers/collaborators_controller_spec.rb b/spec/controllers/collaborators_controller_spec.rb index c7dcaa4b5..858bd3c05 100644 --- a/spec/controllers/collaborators_controller_spec.rb +++ b/spec/controllers/collaborators_controller_spec.rb @@ -37,6 +37,7 @@ end describe CollaboratorsController do before(:each) do + stub_rsync_methods @project = Factory(:project) @another_user = Factory(:user) @update_params = {:user => {:read => {@another_user.id => '1'}}} diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 2ccf820b3..4238babf8 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe GroupsController do before(:each) do + stub_rsync_methods @group = Factory(:group) @another_user = Factory(:user) @create_params = {:group => {:name => 'grp1', :uname => 'un_grp1'}} diff --git a/spec/controllers/members_controller_spec.rb b/spec/controllers/members_controller_spec.rb index 512bbc73c..ab8ab5fbe 100644 --- a/spec/controllers/members_controller_spec.rb +++ b/spec/controllers/members_controller_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe MembersController do before(:each) do + stub_rsync_methods @group = Factory(:group) @user = @group.owner set_session_for @user diff --git a/spec/controllers/personal_repositories_controller_spec.rb b/spec/controllers/personal_repositories_controller_spec.rb index 50ff6f03f..6e020d61e 100644 --- a/spec/controllers/personal_repositories_controller_spec.rb +++ b/spec/controllers/personal_repositories_controller_spec.rb @@ -92,9 +92,6 @@ describe PersonalRepositoriesController do @project.update_attribute(:owner, @user) - @repository.update_attribute(:owner, @user) - @repository.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'admin') - @repository.platform.update_attribute(:owner, @user) @repository.platform.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'admin') end @@ -108,7 +105,7 @@ describe PersonalRepositoriesController do before(:each) do @user = Factory(:user) set_session_for(@user) - @repository.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'reader') + @repository.platform.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'reader') end it_should_behave_like 'personal repository viewer' diff --git a/spec/controllers/repositories_controller_spec.rb b/spec/controllers/repositories_controller_spec.rb index 935de2e40..422fb5128 100644 --- a/spec/controllers/repositories_controller_spec.rb +++ b/spec/controllers/repositories_controller_spec.rb @@ -79,8 +79,8 @@ describe RepositoriesController do before(:each) do @user = Factory(:user) set_session_for(@user) - @repository.update_attribute(:owner, @user) - @repository.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'admin') + @repository.platform.update_attribute(:owner, @user) + @repository.platform.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'admin') end it_should_behave_like 'repository user with owner rights' @@ -90,7 +90,7 @@ describe RepositoriesController do before(:each) do @user = Factory(:user) set_session_for(@user) - @repository.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'reader') + @repository.platform.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'reader') end it_should_behave_like 'repository user with reader rights' diff --git a/spec/factories/repository_factory.rb b/spec/factories/repository_factory.rb index 2188bc17e..4af755b28 100644 --- a/spec/factories/repository_factory.rb +++ b/spec/factories/repository_factory.rb @@ -3,18 +3,12 @@ Factory.define(:repository) do |p| p.description { Factory.next(:string) } p.name { Factory.next(:unixname) } p.association :platform, :factory => :platform - p.association :owner, :factory => :user end -Factory.define(:personal_repository, :class => Repository) do |p| - p.description { Factory.next(:string) } - p.name { Factory.next(:unixname) } - p.association :platform, :factory => :platform - p.association :owner, :factory => :user - - p.after_create { |rep| - rep.platform.platform_type = 'personal' - rep.platform.visibility = 'hidden' - rep.platform.save! +Factory.define(:personal_repository, :parent => :repository) do |p| + p.after_create {|r| + r.platform.platform_type = 'personal' + r.platform.visibility = 'hidden' + r.platform.save! } end diff --git a/spec/models/cancan_spec.rb b/spec/models/cancan_spec.rb index 597d5dd38..248a502d6 100644 --- a/spec/models/cancan_spec.rb +++ b/spec/models/cancan_spec.rb @@ -278,24 +278,19 @@ describe CanCan do context 'with owner rights' do before(:each) do - @repository.update_attribute(:owner, @user) + @repository.platform.update_attribute(:owner, @user) end - [:read, :update, :destroy, :add_project, :remove_project, :change_visibility, :settings].each do |action| + [:read, :create, :update, :destroy, :add_project, :remove_project, :change_visibility, :settings].each do |action| it "should be able to #{action} repository" do @ability.should be_able_to(action, @repository) end end - - it do - @repository.platform.update_attribute(:owner, @user) - @ability.should be_able_to(:create, @repository) - end end context 'with read rights' do before(:each) do - @repository.relations.create!(:object_id => @user.id, :object_type => 'User', :role => 'reader') + @repository.platform.relations.create!(:object_id => @user.id, :object_type => 'User', :role => 'reader') end it "should be able to read repository" do diff --git a/spec/models/comment_for_commit_spec.rb b/spec/models/comment_for_commit_spec.rb index 6f693d03e..446131e5e 100644 --- a/spec/models/comment_for_commit_spec.rb +++ b/spec/models/comment_for_commit_spec.rb @@ -21,6 +21,7 @@ def set_comments_data_for_commit end describe Comment do + before { stub_rsync_methods } context 'for global admin user' do before(:each) do @user = Factory(:admin) diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index b44f85684..d692beb19 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -15,6 +15,7 @@ def set_commentable_data end describe Comment do + before { stub_rsync_methods } context 'for global admin user' do before(:each) do @user = Factory(:admin) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 9056e45c7..c742a5671 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -10,14 +10,9 @@ describe Repository do @params = {:name => 'tst_platform', :description => 'test platform'} end - it 'it should increase Relations.count by 1' do - rep = Repository.new(@params) - rep.platform = @platform - rep.owner = @platform.owner - rep.save! - Relation.by_object(rep.owner).by_target(rep).count.should eql(1) -# (@platform.owner.repositories.where(:platform_id => @platform.id).count == 1).should be_true + it 'it should increase Repository.count by 1' do + rep = Repository.create(@params) {|r| r.platform = @platform} + @platform.repositories.count.should eql(1) end end - #pending "add some examples to (or delete) #{__FILE__}" end diff --git a/spec/models/subscribe_spec.rb b/spec/models/subscribe_spec.rb index eff91b9a8..40ccddd3c 100644 --- a/spec/models/subscribe_spec.rb +++ b/spec/models/subscribe_spec.rb @@ -12,6 +12,7 @@ def set_testable_data end describe Subscribe do + before { stub_rsync_methods } context 'for global admin user' do before(:each) do @user = Factory(:admin)