Use superclass for CSV method

[adrianna-chang-shopify]

Change the implementation of CsvTask to be a class from which application Tasks can inherit from, instead of a mixin.

It was unclear to me whether we're okay keeping the use of respond_to?(csv_content=) in TaskJob when setting csv_content on a Task:

maintenance_tasks/app/jobs/maintenance_tasks/task_job.rb

Lines 59 to 62 in d583546

	@task = Task.named(@run.task_name).new
	if @task.respond_to?(:csv_content=)
	@task.csv_content = @run.csv_file.download
	end

Or whether we wanted to introduce a class-level method like csv_task? instead. Let me know!

[etiennebarrie]

Or whether we wanted to introduce a class-level method like csv_task? instead.

What I don't like about things like csv_task? is that now we have to define it in Task which should not care about CsvTask at all. I think respond_to? is fine.

[adrianna-chang-shopify]

What I don't like about things like csv_task? is that now we have to define it in Task which should not care about CsvTask at all

Yeah, that was my reasoning as well - I don't think the parent class should have to know anything about its subclasses and the specialized behaviour they provide. I'm keen on sticking to our use of respond_to?

[sambostock]

If I'm not mistaken, .respond_to?(:csv_content=)/.csv_task? are used to trigger loading of the CSV contents in TaskJob, right?

if @task.respond_to?(:csv_content=)
  @task.csv_content = @run.csv_file.download
end

If that's it, would it make sense to define a generic hook/callback method on Task, which CsvTask overrides. That way you can invert things and make it so the TaskJob doesn't know anything about CSVs

# task.rb
class Task
  def before_perform(_run)
  end
end

# csv_task.rb
class CsvTask < Task
  def before_perform(run)
    self.csv_content = run.csv_file.download
    super
  end
end

This could alternatively be implemented as an ActiveSupport::Callback.

[etiennebarrie]

would it make sense to define a generic hook/callback method on Task, which CsvTask overrides

It could make sense, but then we would give too much API to Task writers, which don't want to. We provide CsvTask and TaskJob, so I think even if not ideal, it's fine to have some knowledge between them.
Whereas for all the tasks that the users write, we don't want to give them control over the Run object, it's an implementation detail they shouldn't have to worry about.

[adrianna-chang-shopify]

Providing a callback on Task is pretty neat! But yeah, as Étienne mentioned, the tradeoff is making Task aware of Run, which we want to avoid. It's not the prettiest to use respond_to?, I agree, but since TaskJob already knows how to talk to Run and the Task instance it's creating, I believe it's favourable to having Tasks be aware of implementation details.