H12 Lam Pham - Aaron Shin #299
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the "+" button from the CropSelector component in the Harvest form to enforce that crops must be planted before they can be harvested, addressing a workflow inconsistency.
- Added an
allowCreateprop to CropSelector component with default valuetrue - Updated the Harvest form to pass
allowCreate="false"to CropSelector - Modified the component logic to check both user permissions and the new prop before showing the "+" button
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
modules/farm_fd2/src/entrypoints/harvest/App.vue |
Added allowCreate="false" prop to CropSelector to disable crop creation in harvest workflow |
components/CropSelector/CropSelector.vue |
Added new allowCreate prop and updated logic to conditionally hide the "+" button based on this prop |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (this.canCreateCrop && this.allowCreate) { | ||
| this.popupUrl = '/admin/structure/taxonomy/manage/plant_type/add'; | ||
| } else { | ||
| this.popUrl = null; |
There was a problem hiding this comment.
Typo in variable name: popUrl should be popupUrl. This will cause the "+" button to remain visible even when allowCreate is set to false, as the correct variable (popupUrl) won't be set to null.
| this.popUrl = null; | |
| this.popupUrl = null; |
There was a problem hiding this comment.
This change should be made. Currently the solution is working only because this.popupURL is null by default, but not because of this line.
There was a problem hiding this comment.
We delete the unecessary else statement for the popUrl.
braughtg
left a comment
braughtg
left a comment
There was a problem hiding this comment.
Overall this is looking pretty good. There are some changes that have been requested.
In addition, the E2E tests for the Harvest form should confirm that the "+" button is not visible when the CropSelector is used on the Harvest form. The component test requested checks that the new prop works, this test will check that that prop has actually been set by the Harvest form.
Please make the requested changes and push them to this PR. When you have made the changes change the PR back to "Ready for review".
| if (this.canCreateCrop && this.allowCreate) { | ||
| this.popupUrl = '/admin/structure/taxonomy/manage/plant_type/add'; | ||
| } else { | ||
| this.popUrl = null; |
There was a problem hiding this comment.
This change should be made. Currently the solution is working only because this.popupURL is null by default, but not because of this line.
* Fix typo in variable name popupUrl * Add JSDoc documentation for allowCreate prop * Add test case to check the allowCreate prop works
3 participants
Purpose
The
CropSelectorcomponent used in the Harvest form previously included a "+" button that allowed users to add new crop types. This behavior is inconsistent with the workflow, as a crop must exist and be planted before it can be harvested.This PR removes that "+" button to prevent the creation of new crops during the harvest process.
Verification steps
Follow these steps manually to confirm that the fix behaves correctly:
Approach
CropSelectorComponent: Added a new prop calledallowCreatewhich defaults totrueto maintain existing behavior in other forms.canCreate) and this newallowCreateprop before setting thepopupUrl.allowCreateisfalse,popupUrlremainsnull, which signals the underlyingSelectorBaseto hide the "+" add button.modules/farm_fd2/src/entrypoints/harvest/App.vue, passedv-bind:allowCreate="false"to theCropSelectorto explicitly disable crop creation in this specific workflow.Testing
We added a new Cypress component test named
Check that allowCreate prop handles visibility of crop plus button. This test mounts theCropSelectorcomponent with theallowCreateprop set to false and verifies that the "+" button is correctly hidden.Related issues
Closes #279
Additional information
Time spent: ~1 hour
Licensing Certification
FarmData2 is a Free Cultural Work and all accepted contributions are licensed as described in the LICENSE.md file. This requires that the contributor holds the rights to do so. By submitting this pull request I certify that I satisfy the terms of the Developer Certificate of Origin for its contents.