Split TaskData into two separate classes

[adrianna-chang-shopify]

Closes: #675

TaskData has become a combination of two separate interfaces: one for offering high-level representations of a task on the index page, and one for offering more details info on a specific task on the show page. This PR separates the two roles out into different classes, clarifying the responsibility of each object.

I've also dropped the last_run alias from TaskDataIndex, since these tasks have "related runs" instead of "last runs" with the changes introduced in #667.

Open to feedback on naming, I couldn't think of a good way to distinguish the two classes other than the context in which they're used 😅

[nvasilevski]

👍

[paarthmadan]

Feel free to dismiss my review because I certainly lack context in this gem. That being said, I think there's an opportunity to better structure these objects:

The motivation for the refactor

As I understand it, we feel that currently the TaskData abstraction holds too many responsibilities:

1. Defining an API on TaskData to be used in the index view
2. Defining an API on TaskData to be used in the show view
3. Defining the query for available_tasks
4. Defining the query for find

The current solution

The proposed solution introduces a division of responsibility based on the consuming view. That is, we have a separate abstraction for the index view and a separate abstraction for the show view. Both abstractions define the state and also define the query used to aggregate the relevant task(s).

An alternative solution

To me, it seems there's two responsibilities in question

1. Task aggregation (querying Task(s))
2. Task presentation (decorating a Task with helpers for the UI)

If we keep the TaskData class as is, but remove those class methods that are responsible for querying relevant tasks I think it's responsibility is clear. I see the argument that some methods aren't used by #index but, in my view, it's up to the UI to determine what it needs to fulfill its role and its reasonable that each view doesn't make use of the entire API.

Then, we can move those query methods #available_tasks and #find into their own abstractions. For instance, we can introduce an all new TaskPresenter#index, TaskPresenter#show or even separate abstractions for each of the views: TaskIndexPresenter#tasks, TaskShowPresenter#task where these methods return a TaskData instance(s).

Thoughts

I'm curious to hear how you think about this? I'm not sold on the proposed abstractions I presented, but I'm interested in hearing how you're thinking about this separation of responsibility.

[adrianna-chang-shopify]

Hey @paarthmadan ! Thanks for bringing this up ❤️ To me, the current code is violating the Interface Segregation Principle:

In the field of software engineering, the interface segregation principle (ISP) states that no code should be forced to depend on methods it does not use.[1] ISP splits interfaces that are very large into smaller and more specific ones so that clients will only have to know about the methods that are of interest to them.

IMHO it's less about TaskData being overloaded with too many responsibilities, and more about the fact that one set of responsibilities is used in one context (displaying tasks for the index page), and the other set is used in an entirely separate context (displaying tasks on the show page). Part of the reason the original issue was opened was because TaskData objects on the show page have no related run -- so it seems odd for those TaskData instances to have a related_run / last_run attribute that is really has no intent to use.

Your point on separating querying vs presentation responsibilities is an interesting one! We could split out the querying / aggregating behaviour from the actual object that represents a Task in the UI, but to me it would still be fundamental to have TaskPresenter#index and TaskPresenter#show return different objects. And I'm not sure it buys us much at that point to have these two methods pulled out into a separate presenter object, rather than having each of the query methods as a class method in the respective TaskData class.

Does this make sense?

[paarthmadan]

Thanks for engaging in the discussion! ❤️

...but to me it would still be fundamental to have TaskPresenter#index and TaskPresenter#show return different objects.

👍 Your rationale makes sense given that it's important to return different objects.