issue addressed: 4274 #4366
issue addressed: 4274 #4366
Conversation
… existing item. A helpful message pops up stating the error. This is relevant to items within the same organization only.
…n for an already existing item name. This method will return true for items with the same name within the same organization. Will return false if the name does't exist within an organization. Will return false if the name exist but within a different organization.
… scenario of a name already being used. Create private method to abstract the error handling logic from the create method and call the model method that checks if an item name exist.
|
Hey @Gabe-Torres. Thank you for this! |
|
Also be sure to remove the |
…wncase on name parameter for case insensitive scenario
…ore action, check_for_existing_item_name, that triggers the query method to check for matching names before the create action. There is a conditional, item_params_valid?, that will only trigger the before action if the item is valid. This was needed because invalid params were going through the query, causing the "with invalid params" test(ln: 170) to fail. Lastly I made a helper method, prepare_new_action_render, to keep the code DRY.
|
I removed the WIP and it is ready to be reviewed again. It should be noted that I am still getting two recurring |
| @@ -1,6 +1,8 @@ | |||
| # Provides full CRUD to Items. Every item is rooted in a BaseItem, but Diaperbanks have full control to do whatever | |||
| # they like with their own Items. | |||
| class ItemsController < ApplicationController | |||
| before_action :check_for_existing_item_name, only: [:create], if: :item_params_valid? | |||
There was a problem hiding this comment.
I feel like really what we want to do here is have a validation on uniqueness, no? You can only have one item per organization with the same name.
There was a problem hiding this comment.
Ahhhh that makes total sense. There is already a validation on uniqueness, so I will go off that. I initially went that route but ran into some issues. I'm going to try again and I'll be sure to reach out if I have issues!
| flash[:error] = "Something didn't work quite right -- try again?" | ||
| render action: :new | ||
| prepare_new_action_render |
There was a problem hiding this comment.
Not sure I understand why this was moved? All you need to do here is change flash[:error] to be the actual error message instead of a generic "Something didn't work quite right".
There was a problem hiding this comment.
Makes perfect sense, I initially did that but overthought it and convinced myself it was not that simple... 🥲 thank you! I will get this fixed up right now.
| @@ -128,6 +127,46 @@ | |||
| end | |||
| end | |||
|
|
|||
| context "with an already existing item name" do | |||
There was a problem hiding this comment.
We generally use request specs, not controller specs (which are discouraged in new Rails).
| expect(response).to render_template(:new) | ||
| end | ||
|
|
||
| it "shouldn't create an item with the same name, case-insensitively" do |
There was a problem hiding this comment.
This is a superset of the previous test - if it fails case-insensitively, it will by definition also fail for case-sensitive.
Resolves #4274
Description
This PR implements changes to the
ItemsControllerand adds an instance method to theItemmodel to query an organization for existing item names. These changes allow banks to receive a clear message, “An item with that name already exists (could be an inactive item),” when attempting to enter a new item name that already exists.Please also include relevant motivation and context.
Main changes:
rescue_fromwithActiveRecord::RecordInvalidGuide questions:
-- Banks should have a clear message explaining why an item couldn’t be created in the case of creating an item with the same name as another.
-- I considered using
ActiveRecordto handle the case without the new model method but wanted clean code with the desired message.-- Using the model method allowed for easier readability and control over error messages. This allowed to keep the other error message in the case of the error not being matching item names.
-- Using
ActiveRecordcould be a more useful route since it is built in and would reduce the amount of logic in the controller and model.List any dependencies that are required for this change. (gems, js libraries, etc.)
Include anything else we should know about. -->
Type of change
How Has This Been Tested?
Screenshots
I added WIP in the title since I got a couple of failing tests.
Ferrum:: JavaScriptError: TypeError: Cannot read properties of null (reading 'dispatchEvent') at Cuprite,trigger (<anonymous>: 412:10) at Cuprite.select (<anonymous>: 359:12) at HTMLOptionElement. <anonymous> (<anonymous>: 1:31)I believe others have been having similar issues when running the test suite and this is being figured out currently. The two failing test do pass if I run the test separately.
Include screenshots (before / after) for style changes, highlight edited element.-->
Before
After