Move meta related actions from ProjectController to its own CRUD controller

[vpereira]

Move save_meta and meta from ProjectController to Projects::MetaController

• write new code
• adapt the rules to point to the new code
• change the views
• rewrite tests
• cassettes
• delete old code from ProjectController
• refactor the Projects::MetaController#update action

[ChrisBr]

Otherwise nice work 👍 and I like that we reduce the size of some of the big controllers step by step

[ChrisBr]

I very much like the approach of introducing service classes ❤️ 👍 But I would have some suggestions:

• I would prefer to not have dedicated helper methods but do everything in the action (at least for validate_request_and_set_error). We could do the request validation in the ProjectUpdater (composition!)
• [nitpick] I would prefer to name the method on the classes validate and update instead of call. I think especially when using the composition approach this does make sense.
• Maybe we can have a helper method valid? instead of calling empty?.
• The name RequestValidator does not make much sense in this context, what about MetaValidator and the other one MetaXmlValidator

I imagine it could look something like this:

module MetaControllerService
   class ProjectUpdater
     attr_reader :errors

     def initialize(project: nil, request_data = {}, validator = ::MetaControllerService::RequestValidator)
       @project = project
       @request_data = request_data
       @validator = validator.new(project: project, request_data: request_data)
     end

     def update
      unless validator.valid?
        errors = validator.errors
        return
      end
    
       Project.transaction do
         errors = project.update_from_xml(request_data)[:error]
         project.store unless errors?
       end
     end

     def valid?
       validator.valid? && errors.present?
     end

    private

    attr_writer :errors
    attr_reader :project, :request_data
   end
 end

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

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

What do you think? Anyway, really great work 👍

[vpereira]

I very much like the approach of introducing service classes But I would have some suggestions:

Thank you for the extended review. Code examples helps me a lot! <3

One issue that I have doing like that is breaking the Single Responsibility principle, coupling again the classes. In another hand the ProjectUpdater would be a good place to make it. I will have a look, thank you!

[ChrisBr]

One issue that I have doing like that is breaking the Single Responsibility principle, coupling again the classes.

I don't see how this is violation Single Responsibility as this is a classic example of composition. Object orientation is all about sending messages between objects. As long as we only rely on the interface of the validator (validate method) and pass in the validator as an instance variable, this does not violate Single Responsibility and does not couple them as the only thing Updater knows is the public interface validate. On the contrary, this ensures that both ProjectUpdater and Validator keep their responsibility. And keeping the validator as instance variable ensures that you can also test in isolation like:

  it 'is valid' do
    validator = double("validator", valid: true)
    updated = ProjectUpdater.new("foo", "bar" , validator)
    ...
  end

  it 'is invalid' do
    validator = double("validator", valid: false)
    updated = ProjectUpdater.new("foo", "bar" , validator)
    ...
  end

But I see your concern and to make this more clear, you could introduce an intermediate Service class which takes an updater and validator and composes them together like:

class MetaService
  def initialize(updater = ProjectUpdated, validator = MetaValidator)
     # set instance variables
  end

  def save
    validator.valid?
    updater.update
    # etc etc.
  end

But honestly, I think this would be overkill and the initial approach with the two classes is sufficient for now.

This is just pseudo code to visualize my thoughts and is not intended to work right away.

[vpereira]

please check it @ChrisBr

I renamed the request_validator to meta_xml_validator I will keep the call method to the services, since it's a common practice to have it (keeping all Services with a simple and similar API)

I think for now it's ok as it is, I would like to try to submit in this sprint the same changes for prjconf (in another PR)

[ChrisBr]

See my other comment. I would prefer to change this method to a private validate method:

class RequestValidator
  def valid?
    validate
    @errors.empty?
  end

  private

  def validate
    # ... the validation code from previous call method
  end

This has some advantages:

• keeping the interface as small as possible (one method less in the public interface)
• don't need to remember to call call before calling valid?
• keeps the same behaviour than ActiveRecord

[ChrisBr]

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.

[ChrisBr]

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

[ChrisBr]

I still don't think that the class name reflects what this class is doing: MetaValidator would make it more clear I think.

[ChrisBr]

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.

[ChrisBr]

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

[vpereira]

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