| [07:24:08] | * MiriamNugent[m] has joined #farmos |
| [07:24:08] | <MiriamNugent[m]> | Hi mstenta Greg I'm working on a bug that Greg found when using quick forms with farm multitenant installed. When using the built-in Planting quick form we get a validation error when saving the log, because the associated asset isn't assigned to a farm. |
| [07:24:08] | <MiriamNugent[m]> | To solve this, I'm thinking we could add a quick form submodule to multitenant with a QuickForm plugin that extends the built-in planting quick form and adds a mandatory farm field that can be saved to the asset. This works, although phpstan complains because Planting is defined as @internal |
| [07:24:08] | <MiriamNugent[m]> | What do you think? Is this the right approach or should I just document in the README how to create a multitenant quick form? |
| [07:28:38] | <mstenta[m]> | Hmm two thoughts: |
| [07:29:33] | <mstenta[m]> | 1. Maybe core Planting quick form should have a conditionally added field for specifying what farm the plant asset is in, if the farm organization module is installed |
| [07:30:14] | <mstenta[m]> | We should think about the other core quick forms too in the same regard... eg: the Birth quick form |
| [07:30:37] | <mstenta[m]> | In general though, core should not require a farm |
| [07:30:46] | <mstenta[m]> | So that leads to... |
| [07:32:14] | <mstenta[m]> | 2. Does multitentant explicitly or implicitly require the farm to be populated on all assets? Is the validation constraint specific to nultitenant? |
| [07:32:50] | <mstenta[m]> | s/nultitenant/multitenant/ |
| [07:34:58] | <MiriamNugent[m]> | multitenant alters the asset form to make the farm mandatory for users that don't have administer farm_multitenant permission. The validation constraint is specific to multitenant |
| [07:44:43] | <mstenta[m]> | Got it |
| [07:46:01] | <mstenta[m]> | So yea best solution is probably to add a field to the core form and then make that required via multitenant too in form alter |
| [07:47:00] | <mstenta[m]> | Although it does lead to a bigger question about how tenable this is more broadly |
| [07:47:25] | <mstenta[m]> | Multitenant can add form alters for core quick forms |
| [07:47:36] | <mstenta[m]> | But not contrib ones |
| [07:48:12] | <mstenta[m]> | But maybe that will always be a known limitation, and those contrib would be responsible for adding compatibility with multitenqnt |
| [07:48:35] | <mstenta[m]> | Can't think of any specifically... just thinking ahead... |
| [07:49:30] | <mstenta[m]> | mstenta[m]: I'm hoping to get features organized for farmOS 4.1.0... maybe this could be included |
| [07:49:43] | <MiriamNugent[m]> | Maybe multitenant could detect contrib forms that don't have a farm field, and add a warning saying the form is not compatible with multitenant |
| [07:50:14] | <mstenta[m]> | Hmm maybe... might be more complicated than that... |
| [07:51:08] | <mstenta[m]> | eg: the Birth quick form... that also doesn't have a farm field. It should probably automatically assign child assets to the same farm as the mother |
| [07:51:19] | <mstenta[m]> | So it will never have a farm field |
| [07:51:52] | <mstenta[m]> | But yea, maybe we don't need to worry about all hypothetical contrib forms |
| [07:52:05] | <mstenta[m]> | We should ensure it works with core forms though |
| [07:52:45] | <mstenta[m]> | Planting and Birth may be the only ones that create assets... can't remember if others do... 🤔 |
| [07:53:04] | <mstenta[m]> | Most others only create logs |
| [07:53:25] | <MiriamNugent[m]> | I checked the others. It's just planting and birth that are affected |
| [07:53:53] | <mstenta[m]> | OK great |
| [07:54:16] | <mstenta[m]> | Q: do you have budget to work on core PRs for those? |
| [07:54:30] | <mstenta[m]> | If not, you could just temporarily form alter them |
| [07:54:55] | <mstenta[m]> | (of course, that is tech debt that needs to be undone when we do make the core changes) |
| [07:56:03] | <MiriamNugent[m]> | I will have to check with Greg about budget. |
| [07:56:54] | <MiriamNugent[m]> | I looked at doing a form alter, but the form submission is tricky because I would need to add the farm to the asset created by the core submission handler |
| [07:57:35] | <MiriamNugent[m]> | I don't think I would have access to the asset's entity id |
| [07:58:04] | <mstenta[m]> | Oh I see |
| [07:58:40] | <MiriamNugent[m]> | that's why I extended the form instead |
| [08:00:04] | <mstenta[m]> | Maybe we need some mechanism of tracking entities that are created during quick form submit, so that form alter can add another submit that runs after it and pull those from form state... |
| [08:00:17] | <mstenta[m]> | (Actually... maybe we do? 🤔) |
| [08:01:38] | <mstenta[m]> | Ah no... doesn't look like we do |
| [08:01:46] | <mstenta[m]> | https://github.com/farmOS/farmOS/blob/4.x/modules/core/quick/src/Traits/... |
| [08:02:01] | <mstenta[m]> | That is another nice little PR we could consider |
| [08:02:31] | <mstenta[m]> | But lower priority if we're just going to add farm field handling to core anyway |
| [10:08:47] | <barkerd427[m]> | mstenta[m]: Thank you. I will work on this for farmos.py. It's quite necessary with AI writing code but human reviewers. |
| [10:43:51] | <mstenta[m]> | Oops sorry barkerd427 - that was actually out of context. |
| [10:44:00] | <mstenta[m]> | I sent a message to the wrong room 😅 |
| [10:44:38] | <mstenta[m]> | Wait... no I didn't haha - I thought that chat with Miriam Nugent was in a different room. I was on my phone at the time. |
| [10:45:34] | <mstenta[m]> | barkerd427: I see now you were referring to the "stacked PRs" message from earlier. |
| [10:45:54] | <mstenta[m]> | Disregard my confusions... 😅 |
| [11:54:11] | <symbioquine[m]> | The weekly farmOS development call is on in about 6 minutes - all are welcome! https://meet.jit.si/farmos-dev |
| [11:57:30] | <barkerd427[m]> | I'll be 30 minutes late. |