Add action to bulk categorize logs

[paul121]

Originally requested here: Rothamsted-Ecoinformatics/farm_rothamsted#309

It's a pretty simple add that I hope would be quite useful for others as well!

[paul121]

I chose to add this to the farm_log module. This module already hosts our asset_add_log_action.

It's a bit tricky because the log.category base field is added with farm_entity_fields. There is also a separate module for the taxonomy: farm_log_category. I'm curious if we should move the log.category base field to this farm_log module, similar to my PR #588

Aside from that, I believe this PR should include an update hook to add the system.action.categorize_log_action config. The tricky thing is that this is optional config. What is the expected behavior if a module adds new optional config, should it be enabled by default, or should it remain as an option that can be enabled if/when the user wants it? I guess I'm curious what a Drupal core update would do in this situation.

Worth noting that this decision is sorta irrelevant for #588 because we are creating a new module, and when that module is installed, the new action will basically be created on everyone's install, even though it is optional config as well 🤷

My assumption is that we should just enable this by default because most farmOS instances are offered with "managed" hosting and this is the easy option. However, managed hosting providers should also have the ability to add this config themselves if they determine this is best for their users. Overall my brain is mush and I guess mostly just want to acknowledge other hosting situations - requesting review from a few folks that come to mind! 😃 @Farmer-Eds-Shed @symbioquine @SirSundays @pcambra

[paul121]

Aside from that, I believe this PR should include an update hook to add the system.action.categorize_log_action config.

I've added an update hook that creates the system.action.log_categorize_action config entity.

I also changed the machine name of the action from categorize_log_action to log_categorize_action. I think the prior reads a bit better but we have an existing convention where the entity type is the prefix of our action plugin so I went with that for consistency.

[mstenta]

I've added an update hook that creates the system.action.log_categorize_action config entity.

I think that's the right approach.

Only question is: will this break if one of the necessary ("optional") module dependencies is not installed? My understanding is that Drupal installs optional config when the module is installed IF all the optional dependencies are met. Otherwise it does not... and will try again next time one of those dependencies is installed.

I wonder if there's a Drupal core service/method for this? So we can basically say: "Hey Drupal install optional config for this module if you think we should".

Actually looking closer at the changes now... it only depends on farm_log and log... so I think we can just install it. :-)

[mstenta]

It's a bit tricky because the log.category base field is added with farm_entity_fields. There is also a separate module for the taxonomy: farm_log_category. I'm curious if we should move the log.category base field to this farm_log module, similar to my PR #588

Hmm. I suppose we could simply just add a dependency on farm_entity_fields to farm_log... but we should make sure that doesn't create a circular dependency. If it does (and/or if seems like we're creating tangles that will need to be teased apart in the future anyway), then maybe we should move that field to farm_log like you suggest.

[paul121]

I wonder if there's a Drupal core service/method for this? So we can basically say: "Hey Drupal install optional config for this module if you think we should".

This is a great idea! I agree it shouldn't be necessary in this case but something to consider for other updates.

[paul121]

I suppose we could simply just add a dependency on farm_entity_fields to farm_log... but we should make sure that doesn't create a circular dependency.

I see in the UI that farm_entity_fields requires farm_log. Not directly, but it seems that the dependency chain is: farm_entity_fields -> farm_location -> farm_log.

[mstenta]

Drats. I had a feeling there was going to be something like that. :-P

[mstenta]

What if we add it to taxonomy/farm_log_category?

[paul121]

What if we add it to taxonomy/farm_log_category?

We could but doesn't seem like the best fit... I think the action is more about the log.category field than the taxonomy. We could add the action to farm_entity_fields instead?

I wonder, it would almost be nice if we had a farm_{entity} module for each of asset, log, plan.... that way each of these modules could encapsulate the entity base fields as well as related logic/features/actions associated with those base fields in a dedicated module. Only kind of suggesting that though, I know there are a lot of considerations here

[mstenta]

We could but doesn't seem like the best fit... I think the action is more about the log.category field than the taxonomy.

I had the same feeling. Although I'm sort of vacillating between thinking of the modules/asset/*, modules/log/*, modules/taxonomy/* as a) granular modules that ONLY provide a bundle type, and b) "higher level" modules with more purpose built in. For example, the Group asset module provides a lot more than just the group asset type, and the Equipment asset module adds an equipment reference field. So we aren't perfectly consistent in what those "higher level" (aka non-core) modules can be responsible for. In this case adding a category reference field via the module that provides the category taxonomy is equivalent to the equipment module providing an equipment reference field. 🤷

Not sure I'm arguing for it... just thinking out loud and offering one more potential justification.

We could add the action to farm_entity_fields instead?

This might make some sense, but feels like a bit more of a can of worms to me... we should be making that module less complex IMO - and maybe the long term plan should be to simply dissolve it and move all fields to other modules.

I wonder, it would almost be nice if we had a farm_{entity} module for each of asset, log, plan....

This does make some sense... and feels like a familiar pattern to farmOS 1.x. I think I've been avoiding this up until now in hopes that more granular modules could meet our needs. I recall recently putting some work into simplifying the one we do have (farm_log) in hopes of dissolving that too. This is all just preferences though and shuffling deck chairs around perhaps.

I think typing all this out actually has me leaning more towards putting it in farm_log_category, and just accepting that we are thinking of these modules more as:

b) "higher level" modules with more purpose built in

But curious what you think about that. Are there other considerations there? And/or... are there other things that could potentially be moved similarly?

[mstenta]

b) "higher level" modules with more purpose built in

Another way to think of this: if farm_log_category were a contrib module, you would expect it to provide both the taxonomy and add a reference field to log types.

[paul121]

In this case adding a category reference field via the module that provides the category taxonomy is equivalent to the equipment module providing an equipment reference field. shrug

Ah yes, I agree! Maybe I misunderstood above. Adding only the categorize action to farm_log_category seems odd, but if we add both the categorize action and the log.category base field then I agree that makes more sense!

[mstenta]

Ha yea looking back I made that conceptual leap without realizing or expressing it! 😄

[paul121]

Well, happy to go that route! The only issue I can think of would be test dependencies... if there are any tests that use the log.category field, we may need to adjust test dependencies some if they only depend on farm_entity_fields

[mstenta]

I know there's one test in farm_entity that checks if log_category field exists. We can just remove that IMO. It's not a very consequential test. Not sure if there are others...

[paul121]

Okay, I moved the log.category base field to farm_log_category and moved all of the log categorize action stuff over as well. Everything seems to be working fine, I'm just curious if we need an update hook for the log.category field change. Right now the cache must be rebuilt after making this change.

I tested by doing a fresh install on 2.x, creating some logs with categories, and then checking out this branch. Immediately after the status report shows "log category field needs to be uninstalled". After rebuilding the cache once the error is removed from the status report. This seems to be okay but I think ideally we would have some kind of an update hook?

[paul121]

Maybeee related to #583 ?

[mstenta]

Right now the cache must be rebuilt after making this change.

If a cache rebuild is the only thing needed I don't think we also need an update hook.

The official update instructions say to run drush cr after running update.php: https://farmos.org/hosting/update/#update-procedure

So IMO I think we can merge this as-is and call it good. Agreed?

[paul121]

Cool. Sounds good to me

[mstenta]

Merged! Thanks again for the help on this one @paul121!