To be honest, I have lots of doubts with this PR.

For one hand, this will allows us to prevent some of the typical bugs, like "X action isn't in the ActionLog" or "Y command does not generate a Notification".

On another hand, even though there is lots of copy and paste with the current approach (and that for sure generated bugs these kind of bugs), what I like is that it's really easy to understand with one look what's happening there. I'm specially concerned about newcomers to the development and making the learning curve steeper.

I think my main blocker are the changes in some of the Accountability commands. For instance, Decidim::Accountability::Admin::CreateImportedResult.

This is what it looks like until now:

https://github.com/decidim/decidim/blob/b7f9386278e0c2557236ac0c88527ea8546c5796/decidim-accountability/app/commands/decidim/accountability/admin/create_imported_result.rb

I just have to know how Decidim::Command works. As we have links to Rectify and they have some documentation here with examples, then I think it's not difficult to grok. It's not a typical Rails pattern, but it's not that alien neither. Sometimes it's called the ServiceObjects pattern, and searching for a quick example I see an API that's pretty much what we have (TweetCreator.call(params[:message]))

So, going back to Decidim::Accountability::Admin::CreateImportedResult, this is what it looks like with the proposed changes:

https://github.com/decidim/decidim/blob/1678c1b3b4bf77e8a28a93d9c11e22c29d6461e2/decidim-accountability/app/commands/decidim/accountability/admin/create_imported_result.rb

Ok, now I need to know:

1. Decidim::Accountability::Admin::CreateResult
2. Decidim::Commands::CreateResource
3. Decidim::Command

But wait, because if I want to understand CreateResource, I also need to see Decidim::Commands::ResourceHandler, as that's where the fetch_form_attributes/attributes are actually defined :/

Now I need to have in my (tiny) brain much more information. Just to understand the full picture of the attributes, in this particular case, I need to see it in three different parts of two files:

Maybe in cases like this (where we're introducing two levels of inheritance) it makes more sense to have a bit of duplication and not inherit from Decidim::Commands::CreateResource?

In the other cases, when it's just simpler the starting point (like Decidim::Accountability::Admin::CreateTimelineEntry), I think the arguments in favor of removing duplication are winning without any doubt.

I have to admin that you had bad luck because these examples come first in the review (as they're from decidim-accountability and GH orders alphabetically the files xD) but reviewing in a commit by commit basis it makes perfect sense all the different decisions. Just that seeing the final form of these cases is a bit of a wtf.

So, as a final summary: can you remove the inheritance in Decidim::Accountability::Admin::CreateResult and Decidim::Accountability::Admin::UpdateResult??

As we've talked yesterday, I still have doubts if the current approach is good for newcomers. To have a second opinion, I want to ask for @microstudi input in this PR.

Can you give us a few thoughts about this? Do you think this would be easy to understand/extend/hack? I'm particularly worried about the super usage.

Hi @andreslucena
I don't see any problems with this, on the contrary, I think it will facilitate the creation of commands that is rather tedious right know.

One think I would do, yes you read my mind... documentate!
I would add some comments on how the class UpdateResource should be used. In the class itself and in the general docs!

Hi @andreslucena I don't see any problems with this, on the contrary, I think it will facilitate the creation of commands that is rather tedious right know.

One think I would do, yes you read my mind... documentate! I would add some comments on how the class UpdateResource should be used. In the class itself and in the general docs!

I have started some documentation in #11962, in order to have the page where i could document the current changes.

@alecslupu we have a failing spec related to #12157. Can you check it out 🙏🏽 ?