Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move transplant_days field to farm_transplant module #795

Merged
merged 2 commits into from Apr 10, 2024

Conversation

mstenta
Copy link
Member

@mstenta mstenta commented Feb 9, 2024

Currently the farm_plant_type provides a plant_type taxonomy. These plant_type terms have a few fields added to them:

  • crop_family - reference to a term in the crop_family vocabulary
  • companions - reference to one or more terms in the plant_type vocabulary
  • transplant_days - integer representing the number of days between seeding and transplanting
  • maturity_days - integer representing the number of days between seeding and maturity/harvest

I am also proposing we add harvest_days to represent the "harvest window" in #794. (This PR is currently built on top of that that one, but we can separate them if we need to.)

My though behind moving transplant_days to the farm_transplant module is: not all farms deal with transplants. A farm that only deals with direct seeding may not have the transplant log type installed, and this transplant_days field is meaningless to them.

By moving it to the farm_transplant module, this field will only be installed when that module is.

mstenta added a commit to mstenta/farmOS that referenced this pull request Feb 9, 2024
@mstenta mstenta marked this pull request as ready for review February 9, 2024 12:02
@mstenta
Copy link
Member Author

mstenta commented Feb 9, 2024

This change also includes an update hook that will uninstall the field ONLY IF there is no data in the taxonomy_term__transplant_days database table AND the farm_transplant module is not installed.

@mstenta
Copy link
Member Author

mstenta commented Feb 9, 2024

Perhaps the only hanging thread here are the core.entity_form_display.taxonomy_term.plant_type.default and core.entity_view_display.taxonomy_term.plant_type.default configurations.

This branch removes transplant_days from those displays, but does not attempt to add them again via the farm_transplant module. This means that the transplant_days field will be added to plant_type terms when farm_transplant is installed, but we are not specifying where or how it should appear in the form or entity display.

I'm not sure if there is a good solution for this, and this may be why we decided to keep transplant_days in the farm_plant_type module during the transition from v1->v2, rather than split it out at that point.

This reminds me of https://www.drupal.org/project/farm/issues/3190851. I do sort of wish we had taxonomy term bundle plugin support, but adding that would be a breaking change at this point.

@mstenta
Copy link
Member Author

mstenta commented Feb 9, 2024

I'm not sure if there is a good solution for this, and this may be why we decided to keep transplant_days in the farm_plant_type module during the transition from v1->v2, rather than split it out at that point.

With all of that in mind, it may mean we shouldn't merge this PR, if it causes more trouble than it solves. I wanted to open it to get thoughts from others at least... perhaps there's another solution out there.

@mstenta
Copy link
Member Author

mstenta commented Mar 20, 2024

This branch removes transplant_days from those displays, but does not attempt to add them again via the farm_transplant module. This means that the transplant_days field will be added to plant_type terms when farm_transplant is installed, but we are not specifying where or how it should appear in the form or entity display.

I rebased this onto #806, which resolves this issue.

This PR is ready for review now.

@mstenta mstenta requested review from paul121 and wotnak March 20, 2024 11:09
@mstenta
Copy link
Member Author

mstenta commented Mar 20, 2024

In testing this with a few different scenarios I discovered an issue with the update hook... testing a fix for it now...

mstenta added a commit to mstenta/farmOS that referenced this pull request Mar 20, 2024
@mstenta
Copy link
Member Author

mstenta commented Mar 20, 2024

OK I fixed the update hook. It now handles a few different cases pretty elegantly, I think:

If there is any transplant_days data saved in the database, and the farm_transplanting module is not installed, the update hook safely installs it (soft-deleting the field config entities first without deleting the database tables, so that farm_transplanting can install its own config).

If there is NO transplant_days data saved in the database, the field instance and storage config entities are deleted in a way that also deletes the unused database table.

There is still a failing test, so I'll look into that now... but otherwise this seems to be working.

@mstenta
Copy link
Member Author

mstenta commented Mar 20, 2024

There is still a failing test, so I'll look into that now...

Ah this was because the farm_quick_planting module kernel test installs farm_transplanting config, but does not install the field module or farm_plant_type config. Adding those (and related dependencies) fixes the tests.

mstenta added a commit to mstenta/farmOS that referenced this pull request Mar 20, 2024
@mstenta mstenta merged commit ee183af into farmOS:3.x Apr 10, 2024
1 of 2 checks passed
@mstenta mstenta deleted the 3.x-move-transplant-days branch April 10, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants