[frontend] Simplify storage of attributes into backend

We need the _attributes file in the backend for revision
storage, but the only code that actually wrote was the
attribute controller in the api. All attributes created,
updated or deleted through the webui weren't recorded
as revision.

So simplify the storage and let the model handle it. This
way we don't have to duplicate the code too much.

I added a write_attributes to the webui controller though
as the API has tests that the revision is not increased
on commits that do not update values (which is a bit questionable,
but I didn't want to break tests)

src/api/app/controllers/attribute_controller.rb

@@ -164,7 +164,6 @@ def delete_attribute
 
     # exec
     ac.destroy
-    @attribute_container.write_attributes(params[:comment])
     render_ok
   end
 
@@ -201,13 +200,8 @@ def cmd_attribute
     end
 
     # exec
-    changed = false
     req.elements('attribute') do |attr|
-      changed = true if @attribute_container.store_attribute_xml(attr, @binary)
-    end
-    if changed
-      logger.debug "Attributes for #{@attribute_container.class} #{@attribute_container.name} changed, writing to backend"
-      @attribute_container.write_attributes(params[:comment])
+      @attribute_container.store_attribute_xml(attr, @binary)
     end
     render_ok
   end

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

@@ -60,6 +60,7 @@ def update
     authorize @attribute
 
     if @attribute.update(attrib_params)
+      @attribute.write_container_attributes
       redirect_to edit_attribs_path(project: @attribute.project.to_s, package: @attribute.package.to_s, attribute: @attribute.fullname),
                   notice: 'Attribute was successfully updated.'
     else

src/api/app/mixins/has_attributes.rb

@@ -9,12 +9,13 @@ def self.included(base)
   class AttributeSaveError < APIException
   end
 
-  def write_attributes(comment = nil)
+  def write_attributes
+    return unless CONFIG['global_write_through']
     project_name = is_a?(Project) ? name : project.name
     if is_a?(Package)
-      Backend::Api::Sources::Package.write_attributes(project_name, name, User.current.login, render_attribute_axml, comment)
+      Backend::Api::Sources::Package.write_attributes(project_name, name, User.current.login, render_attribute_axml)
     else
-      Backend::Api::Sources::Project.write_attributes(project_name, User.current.login, render_attribute_axml, comment)
+      Backend::Api::Sources::Project.write_attributes(project_name, User.current.login, render_attribute_axml)
     end
   rescue ActiveXML::Transport::Error => e
     raise AttributeSaveError, e.summary
@@ -39,21 +40,17 @@ def store_attribute(namespace, name, values, issues, binary = nil)
     attrib_type = AttribType.find_by_namespace_and_name!(namespace, name)
 
     # update or create attribute entry
-    changed = false
     a = find_attribute(namespace, name, binary)
-    if a.nil?
+    unless a
       # create the new attribute
-      a = Attrib.new(attrib_type: attrib_type, binary: binary)
+      a = Attrib.create(attrib_type: attrib_type, binary: binary)
       a.project = self if is_a? Project
       a.package = self if is_a? Package
-      (a.attrib_type.value_count || 0).times do |i|
-        a.values.build(position: i, value: values[i])
-      end
-      raise AttributeSaveError, a.errors.full_messages.join(', ') unless a.save
-      changed = true
     end
     # write values
-    a.update_with_associations(values, issues) || changed
+    a.update_with_associations(values, issues)
+    return unless a.saved_changes?
+    write_attributes
   end
 
   def find_attribute(namespace, name, binary = nil)

src/api/app/models/attrib.rb

@@ -34,6 +34,8 @@ class Attrib < ApplicationRecord
            :validate_issues,
            :validate_allowed_values_for_attrib_type
 
+  after_commit :write_container_attributes, on: [:create, :destroy]
+
   #### Class methods using self. (public and then private)
   def self.find_by_container_and_fullname(container, fullname)
     container.attribs.find_by(attrib_type: AttribType.find_by_name!(fullname))
@@ -82,33 +84,38 @@ def values_removeable?
       (attrib_type.value_count && (attrib_type.value_count != values.length)) # If value_count != values.length
   end
 
-  def update_with_associations(values = [], issues = [])
-    will_save = false
+  def write_container_attributes
+    # also called if container is deleted...
+    return unless (c = container)
+    c.write_attributes
+  end
 
+  def update_with_associations(values = [], issues = [])
     #--- update issues ---#
+    changed = false
     if issues.map(&:name).sort != self.issues.map(&:name).sort
       logger.debug "Attrib.update_with_associations: Issues for #{fullname} changed, updating."
-      will_save = true
       self.issues.delete_all
       issues.each do |issue|
         self.issues << issue
       end
+      changed = true
     end
 
     #--- update values ---#
     if values != self.values.map(&:value)
       logger.debug "Attrib.update_with_associations: Values for #{fullname} changed, updating."
-      will_save = true
       self.values.delete_all
       position = 1
       values.each do |val|
-        self.values.create(value: val, position: position)
+        self.values.build(value: val, position: position)
         position += 1
       end
+      changed = true
     end
 
-    save! if will_save
-    will_save
+    save!
+    saved_changes? || changed
   end
 
   #### Alias of methods

src/api/lib/backend/api/sources/package.rb

@@ -18,9 +18,8 @@ def self.attributes(project_name, package_name, options = {})
 
         # Writes the content in xml for attributes
         # @return [String]
-        def self.write_attributes(project_name, package_name, user_login, content, comment)
+        def self.write_attributes(project_name, package_name, user_login, content)
           params = { meta: 1, user: user_login }
-          params[:comment] = comment if comment
           http_put(['/source/:project/:package/_attribute', project_name, package_name || '_project'],
                    data: content, params: params)
         end

src/api/lib/backend/api/sources/project.rb

@@ -16,9 +16,8 @@ def self.attributes(project_name, revision)
 
         # Writes the xml for attributes
         # @return [String]
-        def self.write_attributes(project_name, user_login, content, comment)
+        def self.write_attributes(project_name, user_login, content)
           params = { meta: 1, user: user_login }
-          params[:comment] = comment if comment
           http_put(['/source/:project/_project/_attribute', project_name], data: content, params: params)
         end
 

src/api/spec/factories/project.rb

@@ -142,9 +142,10 @@
           CONFIG['global_write_through'] ? project.store : project.save!
         end
         if evaluator.create_patchinfo
+          old_user = User.current
           User.current = evaluator.maintainer
           Patchinfo.new.create_patchinfo(project.name, nil, comment: 'Fake comment', force: true)
-          User.current = nil
+          User.current = old_user
         end
       end
 

src/api/spec/features/webui/attributes_spec.rb

@@ -3,7 +3,6 @@
 RSpec.feature 'Attributes', type: :feature, js: true do
   let!(:user) { create(:confirmed_user) }
   let!(:attribute_type) { create(:attrib_type) }
-  let!(:attribute) { create(:attrib, project_id: user.home_project.id) }
 
   def add_attribute_with_values(package = nil)
     visit index_attribs_path(project: user.home_project_name, package: package.try(:name))
@@ -29,6 +28,7 @@ def add_attribute_with_values(package = nil)
   describe 'for a project without packages' do
     scenario 'add attribute with values' do
       login user
+      create(:attrib, project_id: user.home_project.id)
 
       add_attribute_with_values
       expect(page).to have_content('Attribute was successfully updated.')
@@ -74,6 +74,7 @@ def add_attribute_with_values(package = nil)
 
     scenario 'remove attribute' do
       login user
+      attribute = create(:attrib, project_id: user.home_project.id)
 
       visit index_attribs_path(project: user.home_project_name)
 

src/api/spec/features/webui/maintenance_workflow_spec.rb

@@ -23,7 +23,6 @@
   before do
     User.current = admin_user
     create(:maintenance_project_attrib, project: maintenance_project)
-    User.current = nil
   end
 
   scenario 'maintenance workflow' do

src/api/spec/models/attrib_spec.rb

@@ -1,9 +1,10 @@
 require 'rails_helper'
 
 RSpec.describe Attrib, type: :model do
-  let(:attribute) { create(:attrib, project: create(:project)) }
+  let(:user) { create(:confirmed_user) }
   let(:project) { create(:project) }
   let(:package) { create(:package) }
+  let(:attribute) { build(:attrib, project: create(:project)) }
 
   describe '#fullname' do
     it { expect(attribute.fullname).to eq("#{attribute.namespace}:#{attribute.name}") }
@@ -15,9 +16,11 @@
     end
 
     context 'attribute with package' do
-      let(:attribute_with_package) { create(:attrib, package: package) }
-
-      it { expect(attribute_with_package.container).to eq(package) }
+      it 'saves the proper container' do
+        login user
+        attribute_with_package = create(:attrib, package: package)
+        expect(attribute_with_package.container).to eq(package)
+      end
     end
   end
 
@@ -65,27 +68,34 @@
 
   describe '#project' do
     context 'attribute with project' do
-      let(:attribute_with_project) { create(:attrib, project: project) }
+      let(:attribute_with_project) { build(:attrib, project: project) }
 
       it { expect(attribute_with_project.project).to eq(project) }
     end
 
     context 'attribute with package' do
-      let(:attribute_with_package) { create(:attrib, package: package) }
-
-      it { expect(attribute_with_package.project).to eq(package.project) }
+      it 'has the proper project' do
+        login user
+        attribute_with_package = create(:attrib, package: package)
+        expect(attribute_with_package.project).to eq(package.project)
+      end
     end
   end
 
   describe '#update_with_associations' do
     context 'without issues and without values' do
+      before do
+        login user
+        attribute.save
+      end
+
       it { expect(attribute.update_with_associations).to be false }
 
       context 'add an issue' do
         let(:issue_tracker) { create(:issue_tracker) }
         let(:issue) { create(:issue, issue_tracker_id: issue_tracker.id) }
         let(:attrib_type_issue) { create(:attrib_type, issue_list: true) }
-        let(:attribute_with_type_issue) { create(:attrib, project: project, attrib_type: attrib_type_issue) }
+        let(:attribute_with_type_issue) { build(:attrib, project: project, attrib_type: attrib_type_issue) }
 
         subject { attribute_with_type_issue.update_with_associations([], [issue]) }
 
@@ -118,6 +128,7 @@
 
   describe 'validations' do
     before do
+      login user
       subject.valid?
     end
 

src/api/spec/models/bs_request_action/differ/query_builder_spec.rb

@@ -73,8 +73,7 @@
     end
 
     context 'with a maintenance release target project' do
-      let!(:maintenance_release_project) { create(:update_project, name: 'maintenance_project') }
-      let!(:maintenance_package) { create(:package, project: maintenance_release_project, name: 'the_package') }
+      let(:maintenance_release_project) { create(:update_project, name: 'maintenance_project') }
 
       let(:differ) do
         BsRequestAction::Differ::QueryBuilder.new(
@@ -84,9 +83,13 @@
           source_package: source_package.name
         ).build
       end
-      it { expect(differ[:oproject]).to eq('maintenance_project') }
-      it { expect(differ[:opackage]).to eq('the_package') }
-      it { expect(differ.keys.length).to eq(4) }
+      it 'has proper diff' do
+        login user
+        create(:package, project: maintenance_release_project, name: 'the_package')
+        expect(differ[:oproject]).to eq('maintenance_project')
+        expect(differ[:opackage]).to eq('the_package')
+        expect(differ.keys.length).to eq(4)
+      end
     end
   end
 end

src/api/spec/models/user_spec.rb

@@ -105,6 +105,7 @@
     end
 
     it 'will have owned projects and packages' do
+      login user
       create(:attrib, attrib_type: AttribType.find_by(name: 'OwnerRootProject'), project: project_with_package)
       create(:relationship_package_user, package: project_with_package.packages.first, user: user)
       create(:relationship_project_user, project: project_with_package, user: user)

src/api/test/functional/attributes_test.rb

@@ -306,6 +306,7 @@ def test_attrib_delete_permissions
     login_king
     data = "<attributes><attribute namespace='OBS' name='VeryImportantProject'/></attributes>"
     post '/source/home:tom/_attribute', params: data
+    assert_response :success
 
     login_tom
     delete '/source/home:tom/_attribute/OBS:VeryImportantProject'