Merge pull request #1051 from bgeuken/relationship

[Group refactoring] Relationship model

src/api/app/mixins/has_relationships.rb

@@ -204,12 +204,12 @@ def update_generic_relationships(xmlhash)
           pcache[role.title] = :keep
         else
           #new role
-          record = self.relationships.build(role: role)
+          record = self.relationships.new(role: role)
           @updater.set_item(record, item)
           pcache[role.title] = :new
         end
       else
-        record = self.relationships.build(role: role)
+        record = self.relationships.new(role: role)
         @updater.set_item(record, item)
         cache[id] = { role.title => :new }
       end

src/api/app/models/relationship.rb

@@ -1,4 +1,8 @@
 class Relationship < ActiveRecord::Base
+  class SaveError < APIException; end
+
+  FORBIDDEN_PROJECT_IDS_CACHE_KEY="forbidden_project_ids"
+
   belongs_to :role
 
   # only one is true
@@ -11,55 +15,43 @@ class Relationship < ActiveRecord::Base
 
   validates :role, presence: true
 
-  validate :check_sanity
+  validate :check_global_role
+
+  validates_uniqueness_of :project_id, {
+    scope: [:role_id, :group_id, :user_id], allow_nil: true,
+    message: "Project has non unique id"
+  }
+  validates_uniqueness_of :package_id, {
+    scope: [:role_id, :group_id, :user_id], allow_nil: true,
+    message: "Package has non unique id"
+  }
+
+  validates :package, presence: {
+    message: "Neither package nor project exists"
+  }, unless: 'project.present?'
+  validates :package, absence: {
+    message: "Package and project can not exist at the same time"
+  }, if: 'project.present?'
+
+  validates :user, presence: {
+    message: "Neither user nor group exists"
+  }, unless: 'group.present?'
+  validates :user, absence: {
+    message: "User and group can not exist at the same time"
+  }, if: 'group.present?'
 
   # don't use "is not null" - it won't be in index
   scope :projects, -> { where("project_id is not null") }
   scope :packages, -> { where("package_id is not null") }
   scope :groups, -> { where("group_id is not null") }
   scope :users, -> { where("user_id is not null") }
 
-  protected
-  def check_sanity
-    if self.package && self.project
-      errors.add(:package_id, "Relationships are either for project or package")
-    end
-    if self.group && self.user
-      errors.add(:user_id, "Relationships are either for groups or users")
-    end
-    if !self.package && !self.project
-      errors.add(:package_id, "Relationships need either a project or a package")
-    end
-    if !self.group && !self.user
-      errors.add(:user_id, "Relationships need either a group or a user")
-    end
-    check_duplicates if errors.empty?
-  end
-
-  def check_duplicates
-    relation=Relationship.where(role_id: self.role_id)
-    if self.group_id
-      relation=relation.where(group_id: self.group_id)
-    else
-      return unless self.user_id # nothing to check
-      relation=relation.where(user_id: self.user_id)
-    end
-    if self.project_id
-      relation=relation.where(project_id: self.project_id)
-    else
-      return unless self.package_id # nothing to check
-      relation=relation.where(package_id: self.package_id)
-    end
-    if self.id
-      relation = relation.where("id <> #{self.id}")
-    end
-    if relation.exists?
-      errors.add(:role, "Relationship already exists")
-    end
-  end
+  # we only care for project<->user relationships, but the cache is not *that* expensive
+  # to recalculate
+  after_create 'Relationship.discard_cache'
+  after_rollback 'Relationship.discard_cache'
+  after_destroy 'Relationship.discard_cache'
 
-  class SaveError < APIException;
-  end
 
   def self.add_user(obj, user, role, ignoreLock=nil, check=nil)
     obj.check_write_access!(ignoreLock)
@@ -117,8 +109,6 @@ def self.add_group(obj, group, role, ignoreLock=nil, check=nil)
     r.delete if r.invalid?
   end
 
-  FORBIDDEN_PROJECT_IDS_CACHE_KEY="forbidden_project_ids"
-
   # this is to speed up secure Project.find
   def self.forbidden_project_ids
     if User.current
@@ -158,10 +148,11 @@ def self.discard_cache
     User.current.discard_cache if User.current
   end
 
-  # we only care for project<->user relationships, but the cache is not *that* expensive
-  # to recalculate
-  after_create 'Relationship.discard_cache'
-  after_rollback 'Relationship.discard_cache'
-  after_destroy 'Relationship.discard_cache'
+  private
 
+  def check_global_role
+    return unless self.role && self.role.global
+    errors.add(:base,
+               "global role #{self.role.title} is not allowed.")
+  end
 end

src/api/test/unit/package_test.rb

@@ -202,6 +202,7 @@ def test_add_user
   test "names are case sensitive" do
     np = @package.project.packages.new(name: 'testpack')
     xh = Xmlhash.parse(@package.to_axml)
+    np.save!
     np.update_from_xml(xh)
     assert_equal np.name, 'testpack'
     assert np.id > 0

src/api/test/unit/webui/project_helper_test.rb

@@ -5,17 +5,17 @@ class Webui::ProjectHelperTest < ActiveSupport::TestCase
 
   def test_patchinfo_rating_color
     color = patchinfo_rating_color('important')
-    color.must_equal 'red'
+    assert_equal 'red', color
   end
 
   def test_patchinfo_category_color
     color = patchinfo_category_color('security')
-    color.must_equal 'maroon'
+    assert_equal 'maroon', color
   end
 
   def test_request_state_icon
-    map_request_state_to_flag('new').must_equal 'flag_green'
-    map_request_state_to_flag(nil).must_equal ''
+    assert_equal map_request_state_to_flag('new'), 'flag_green'
+    assert_equal map_request_state_to_flag(nil),  ''
   end
 
 end