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

Inventory quick form #766

Merged
merged 11 commits into from
Feb 2, 2024
Merged

Inventory quick form #766

merged 11 commits into from
Feb 2, 2024

Conversation

mstenta
Copy link
Member

@mstenta mstenta commented Jan 4, 2024

This adds an Inventory quick form. For more details on the design and discussion, refer to the following links:

Copy link
Member

@paul121 paul121 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting an error when I try to configure the units on the quick form with a new term that does not exist. It gets a 500 error with this log message:

InvalidArgumentException: The configuration property settings.units.entity doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 76 of /var/www/html/web/core/lib/Drupal/Core/Config/Schema/ArrayElement.php).

@mstenta
Copy link
Member Author

mstenta commented Jan 31, 2024

I'm getting an error when I try to configure the units on the quick form with a new term that does not exist.

Oh good catch @paul121!

This made me think of something related: maybe we should store the term label instead of the term ID in the configuration entity for inventory quick forms. This would allow the config YML to be shared between farmOS instances without any hard-coded term IDs. The term can be generated on-demand using our QuickTermTrait::createOrLoadTerm() method.

I've pushed a commit to that effect. Requesting your review...

I also rebased onto latest 3.x and added a dependency on farm_quick that was missing before.

@mstenta mstenta requested a review from paul121 January 31, 2024 11:10
mstenta added a commit to mstenta/farmOS that referenced this pull request Jan 31, 2024
mstenta added a commit to mstenta/farmOS that referenced this pull request Jan 31, 2024
@@ -494,7 +496,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
*/
public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
$this->configuration['asset'] = $form_state->getValue('asset');
$this->configuration['units'] = $form_state->getValue('units');
$this->configuration['units'] = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would end up saving an empty string if nothing was provided?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I suppose it should explicitly set it to NULL if that's the new default. I'll change it to NULL.

We do need to explicitly set it here otherwise there's no way to "unset" units in configuration.

mstenta added a commit to mstenta/farmOS that referenced this pull request Feb 1, 2024
mstenta added a commit to mstenta/farmOS that referenced this pull request Feb 2, 2024
@mstenta mstenta merged commit 1048852 into farmOS:3.x Feb 2, 2024
2 checks passed
@mstenta mstenta deleted the 3.x-quick-inventory branch February 3, 2024 00:58
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.

2 participants