Merge pull request #1707 from bgeuken/cleanup

Handle backend errors when storing DoD repositories and some small cleanups

src/api/app/controllers/webui/download_on_demand_controller.rb

@@ -4,34 +4,57 @@ class Webui::DownloadOnDemandController < Webui::WebuiController
   def create
     @download_on_demand = DownloadRepository.new(permitted_params)
     authorize @download_on_demand
-    if @download_on_demand.save
-      @project.store
-      redirect_to project_repositories_path(@project), notice: "Successfully created Download on Demand"
-    else
-      redirect_to :back, error: "Download on Demand can't be created: #{@download_on_demand.errors.full_messages.to_sentence}"
+
+    begin
+      ActiveRecord::Base.transaction do
+        RepositoryArchitecture.first_or_create!(
+          repository:   @download_on_demand.repository,
+          architecture: Architecture.find_by_name(permitted_params[:arch])
+        )
+        @download_on_demand.save!
+        @project.store
+      end
+    rescue ActiveRecord::RecordInvalid, ActiveXML::Transport::Error => exception
+      redirect_to :back, error: "Download on Demand can't be created: #{exception.message}"
+      return
     end
+
+    redirect_to project_repositories_path(@project), notice: "Successfully created Download on Demand"
   end
 
   def update
     @download_on_demand = DownloadRepository.find(params[:id])
     authorize @download_on_demand
-    if @download_on_demand.update_attributes(permitted_params)
-      @project.store
-      redirect_to project_repositories_path(@project), notice: "Successfully updated Download on Demand"
-    else
-      redirect_to :back, error: "Download on Demand can't be updated: #{@download_on_demand.errors.full_messages.to_sentence}"
+
+    begin
+      ActiveRecord::Base.transaction do
+        # FIXME: Handle changing architecture or permit it
+        @download_on_demand.update_attributes!(permitted_params)
+        @project.store
+      end
+    rescue ActiveRecord::RecordInvalid, ActiveXML::Transport::Error => exception
+      redirect_to :back, error: "Download on Demand can't be updated: #{exception.message}"
+      return
     end
+
+    redirect_to project_repositories_path(@project), notice: "Successfully updated Download on Demand"
   end
 
   def destroy
     @download_on_demand = DownloadRepository.find(params[:id])
     authorize @download_on_demand
-    if @download_on_demand.destroy
-      @project.store
-      redirect_to project_repositories_path(@project), notice: "Successfully removed Download on Demand"
-    else
-      redirect_to :back, error: "Download on Demand can't be removed: #{@download_on_demand.errors.full_messages.to_sentence}"
+
+    begin
+      ActiveRecord::Base.transaction do
+        # FIXME: should remove repo arch?
+        @download_on_demand.destroy!
+        @project.store
+      end
+    rescue ActiveRecord::RecordInvalid, ActiveXML::Transport::Error => exception
+      redirect_to :back, error: "Download on Demand can't be removed: #{exception.message}"
     end
+
+    redirect_to project_repositories_path(@project), notice: "Successfully removed Download on Demand"
   end
 
   private

src/api/app/helpers/model_helper.rb

@@ -10,14 +10,13 @@ module ModelHelper
   def sync_hash_with_model(entry_class, dblist, inhasharray)
     keys = entry_class._sync_keys
     entries = {}
-    to_delete = {}
 
     dblist.each do |e|
       key = ""
-      keys.each{|k| key << "#{e.send(k)}::"}
-      entries[key]=e
+      keys.each { |k| key << "#{e.send(k)}::" }
+      entries[key] = e
     end
-    to_delete=entries.clone
+    to_delete = entries.clone
 
     entry_class.transaction do
       inhasharray.each do |hash|
@@ -28,12 +27,12 @@ def sync_hash_with_model(entry_class, dblist, inhasharray)
         end
         if entries[key]
           # exists, do we need to update it?
-          modified=nil
-          hash.each do |entry|
-            next if keys.include? entry.first
-            if entry.last != entries[key][entry.first]
-              entries[key][entry.first] = entry.last
-              modified=true
+          modified = nil
+          hash.each do |entry_key, entry_value|
+            next if keys.include?(entry_key)
+            if entry_value != entries[key][entry_key]
+              entries[key][entry_key] = entry_value
+              modified = true
             end
           end
           entries[key].save if modified

src/api/app/models/architecture.rb

@@ -11,8 +11,8 @@ class Architecture < ActiveRecord::Base
   has_many :flags
 
   #### Callbacks macros: before_save, after_save, etc.
-  after_save 'Architecture.discard_cache'
-  after_destroy 'Architecture.discard_cache'
+  after_save :discard_cache
+  after_destroy :discard_cache
 
   #### Scopes (first the default_scope macro if is used)
   scope :available, -> { where(available: 1) }
@@ -23,17 +23,13 @@ class Architecture < ActiveRecord::Base
 
   #### Class methods using self. (public and then private)
 
-  def self.discard_cache
+  def discard_cache
     Rails.cache.delete("archcache")
   end
 
   def self.archcache
-    return Rails.cache.fetch("archcache") do
-      ret = Hash.new
-      Architecture.all.each do |arch|
-        ret[arch.name] = arch
-      end
-      ret
+    Rails.cache.fetch("archcache") do
+      Architecture.all.map { |arch| [arch.name, arch] }.to_h
     end
   end
 

src/api/app/models/download_repository.rb

@@ -5,10 +5,19 @@ class DownloadRepository < ActiveRecord::Base
 
   validates :repository_id, presence: true
   validates :arch, uniqueness: { scope: :repository_id }, presence: true
-  validates :arch, inclusion: { in: Architecture.all.pluck(:name) }
+  validate :architecture_inclusion
   validates :url, presence: true
   validates :repotype, presence: true
   validates :repotype, inclusion: { in: REPOTYPES }
 
   delegate :to_s, to: :id
+
+  def architecture_inclusion
+    # Workaround for rspec validation test (validate_presence_of(:repository_id))
+    if self.repository
+      unless self.repository.architectures.pluck(:name).include?(self.arch)
+        errors.add(:base, "Architecture has to be available via repository association.")
+      end
+    end
+  end
 end

src/api/app/models/repository.rb

@@ -31,6 +31,34 @@ class Repository < ActiveRecord::Base
 
   validates :db_project_id, presence: true
 
+  # FIXME: Don't lie, it's find_or_create_by_project_and_name_if_project_is_remote
+  def self.find_by_project_and_name( project, repo )
+    result = not_remote.joins(:project).where(:projects => {:name => project}, :name => repo).first
+    return result unless result.nil?
+
+    # no local repository found, check if remote repo possible
+
+    local_project, remote_project = Project.find_remote_project(project)
+    if local_project
+      return local_project.repositories.find_or_create_by(name: repo, remote_project_name: remote_project)
+    end
+
+    return nil
+  end
+
+  def self.find_by_project_and_path( project, path )
+    not_remote.joins(:path_elements).where(:project => project, :path_elements => {:link => path})
+  end
+
+  def self.deleted_instance
+    repo = Repository.find_by_project_and_name( "deleted", "deleted" )
+    return repo unless repo.nil?
+
+    # does not exist, so let's create it
+    project = Project.deleted_instance
+    project.repositories.find_or_create_by!(name: "deleted")
+  end
+
   def cleanup_before_destroy
     # change all linking repository pathes
     self.linking_repositories.each do |lrep|
@@ -71,36 +99,6 @@ def project_name
     self.remote_project_name
   end
 
-  class << self
-    # FIXME: Don't lie, it's find_or_create_by_project_and_name_if_project_is_remote
-    def find_by_project_and_name( project, repo )
-      result = not_remote.joins(:project).where(:projects => {:name => project}, :name => repo).first
-      return result unless result.nil?
-
-      # no local repository found, check if remote repo possible
-
-      local_project, remote_project = Project.find_remote_project(project)
-      if local_project
-        return local_project.repositories.find_or_create_by(name: repo, remote_project_name: remote_project)
-      end
-
-      return nil
-    end
-
-    def find_by_project_and_path( project, path )
-      not_remote.joins(:path_elements).where(:project => project, :path_elements => {:link => path})
-    end
-
-    def deleted_instance
-      repo = Repository.find_by_project_and_name( "deleted", "deleted" )
-      return repo unless repo.nil?
-
-      # does not exist, so let's create it
-      project = Project.deleted_instance
-      project.repositories.find_or_create_by!(name: "deleted")
-    end
-  end
-
   def expand_all_repositories
     repositories = [self]
     # add all linked and indirect linked repositories

src/api/app/views/webui/project/repositories.haml

@@ -0,0 +1,20 @@
+- @pagetitle = "Repositories of #{@project.name}"
+- @metarobots = "noindex"
+- project_bread_crumb "Repositories"
+
+= render partial: "tabs"
+
+%h3
+  = @pagetitle
+%p
+  You can configure individual flags for this project here.
+
+- @project.repositories.each do |repository|
+  = render partial: "repository_entry", locals: { repository: repository }
+
+- if User.current.can_modify_project?(@project)
+  %p
+    = link_to(sprite_tag("drive_add", title: "Add repository"), action: "add_repository_from_default_list", project: @project)
+    = link_to("Add repositories", action: "add_repository_from_default_list", project: @project)
+
+= render partial: "shared/repositories", locals: { obj: @project }

src/api/spec/controllers/webui/download_on_demand_controller_spec.rb

@@ -69,7 +69,7 @@
       end
 
       it { is_expected.to redirect_to(root_path) }
-      it { expect(flash[:error]).to eq("Download on Demand can't be created: Arch can't be blank and Arch is not included in the list") }
+      it { expect(flash[:error]).to eq("Download on Demand can't be created: Validation failed: Architecture can't be blank") }
       it { expect(assigns(:download_on_demand)).to be_kind_of(DownloadRepository) }
       it { expect(DownloadRepository.where(dod_parameters[:download_repository])).not_to exist }
     end
@@ -112,15 +112,11 @@
   describe "POST update" do
     let(:dod_repository) { create(:download_repository) }
 
-    before do
-      repository.download_repositories << dod_repository
-    end
-
     context "for non-admin users" do
       before do
         login(create(:confirmed_user))
         dod_parameters[:id] = dod_repository.id
-        dod_parameters[:download_repository][:arch] = "s390x"
+        dod_parameters[:download_repository][:url] = "http://opensuse.org"
 
         post :update, dod_parameters
       end
@@ -129,25 +125,23 @@
       it { expect(flash[:error]).to eq("Sorry, you are not authorized to update this DownloadRepository.") }
 
       it "updates the DownloadRepository" do
-        expect(dod_repository.arch).to eq("x86_64")
+        expect(dod_repository.url).to eq("http://suse.com")
       end
     end
 
     context "valid requests" do
       before do
         login(admin_user)
         dod_parameters[:id] = dod_repository.id
-        dod_parameters[:download_repository][:arch] = "s390x"
+        dod_parameters[:download_repository][:url] = "http://opensuse.org"
+        dod_parameters[:download_repository][:repository_id] = dod_repository.repository.id
 
         post :update, dod_parameters
       end
 
       it { is_expected.to redirect_to(project_repositories_path(project)) }
       it { expect(flash[:notice]).to eq("Successfully updated Download on Demand") }
-
-      it "updates the DownloadRepository" do
-        expect(dod_repository.reload.arch).to eq("s390x")
-      end
+      it { expect(dod_repository.reload.url).to eq("http://opensuse.org") }
     end
 
     context "invalid requests" do

src/api/spec/factories/download_repository_factory.rb

@@ -4,5 +4,12 @@
     url "http://suse.com"
     repotype "rpmmd"
     repository
+
+    before(:create) do |download_repository|
+      RepositoryArchitecture.first_or_create!(
+        repository:   download_repository.repository,
+        architecture: Architecture.find_by_name("x86_64")
+      )
+    end
   end
 end

src/api/spec/models/download_repository_spec.rb

@@ -2,17 +2,20 @@
 
 RSpec.describe DownloadRepository do
   describe "validations" do
+    subject(:download_repository) { create(:download_repository) }
     it { is_expected.to validate_presence_of(:url) }
     it { is_expected.to validate_presence_of(:arch) }
     it { is_expected.to validate_presence_of(:repotype) }
     it { is_expected.to validate_presence_of(:repository_id) }
-    it { expect(create(:download_repository)).to validate_uniqueness_of(:arch).scoped_to(:repository_id) }
+    it { is_expected.to validate_uniqueness_of(:arch).scoped_to(:repository_id) }
     it { is_expected.to validate_inclusion_of(:repotype).in_array(["rpmmd", "susetags", "deb", "arch", "mdk"]) }
 
-    it "validates that architecture is supported by scheduler" do
-      # FIXME: This is required because of seeding our test DB
-      architectures = Architecture.all.pluck(:name)
-      is_expected.to validate_inclusion_of(:arch).in_array(architectures)
+    describe "architecture_inclusion validation" do
+      subject(:download_repository) { create(:download_repository) }
+      it {
+        expect { download_repository.update_attributes!(arch: "s390x") }.to raise_error(
+          ActiveRecord::RecordInvalid, "Validation failed: Architecture has to be available via repository association.")
+      }
     end
   end
 end

src/api/spec/models/project_spec.rb

@@ -224,6 +224,7 @@
                   <master url='http://master.opensuse.org' sslfingerprint='my_fingerprint'/>
                   <pubkey>my_pubkey</pubkey>
                 </download>
+                <arch>i586</arch>
               </repository>
             </project>
           EOF

src/api/test/functional/webui/download_on_demand_controller_test.rb

@@ -88,7 +88,7 @@ def test_adding_download_on_demand
     select('rpmmd', from: 'Type')
     fill_in('Url', with: '')
     click_button('Add Download on Demand')
-    find(:id, 'flash-messages').must_have_text("Download on Demand can't be created: Url can't be blank")
+    find(:id, 'flash-messages').must_have_text("Download on Demand can't be created: Validation failed: Url can't be blank")
   end
 
   def test_editing_download_on_demand
@@ -132,7 +132,7 @@ def test_editing_download_on_demand
     # Fill in the form and send a not working dod data
     fill_in('Url', with: '')
     click_button('Update Download on Demand')
-    find(:id, 'flash-messages').must_have_text("Download on Demand can't be updated: Url can't be blank")
+    find(:id, 'flash-messages').must_have_text("Download on Demand can't be updated: Validation failed: Url can't be blank")
     page.must_have_link 'http://somerandomurl_2.es'
   end