4274- improve error handling on new item

[Gabe-Torres]

Resolves #4274

Description

This PR implements changes to the ItemsController and adds "case-insensitive: false" to the name uniqueness validation in the Item model. 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 and is not case sensitive.

Main changes:

• ItemsController Slightly restructured the create method by changing flash[:error] = "Something didn't work quite right -- try again?" to flash[:error] = "An item with that name already exists (could be an inactive item)."

• Item Model Added case_sensitive: false to name validation within organization scope.

Guide questions: What motivated this change (if not already described in an issue)? -- 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. What alternative solutions did you consider? -- I considered using a custom before_action that uses a query method within the item model but this direction was much easier. (thanks @dorner !) What are the tradeoffs for your solution? -- ActiveRecord is built in rails and reduces the amount of logic in the controller and model. Its much cleaner and easier to read.

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I created a describe block within the items_request_spec on line 80. It makes an item then attempts to make the same item again but lowercase. It expects the item count to not change and the new flash error to render. Do we need to do anything else to verify your changes? Yes, in my previous PR I created my test within the item_controller_spec. Is my test in the correct place or should I move it elsewhere?

This is my first issue and I decided to create a new branch and redo my attempt at this issue so I can have the newest main. There is a current problem with bin/setup that has already been addressed so I haven't been able to test the entire test suite yet. But all test pass within the Items_request_spec I put WIP in the title for this reason.

Screenshots

Before


After


[dorner]

@Gabe-Torres if this is still WIP, please mark it as draft. If not, please remove the WIP.

[Gabe-Torres]

@Gabe-Torres if this is still WIP, please mark it as draft. If not, please remove the WIP.

Gotcha! Removed it. I had put WIP since I couldn't setup the environment with bin/setup at the time

[Gabe-Torres]

After rebasing, and running bin/setup on my local, all tests pass but one in item_system_spec on line 25 1) Item management can create a new item with empty attributes as a user Failure/Error: expect(page.find(".alert")).to have_content "didn't work" expected to find text "didn't work" in "An item with that name already exists (could be an inactive item)."

I'm not sure why the new error message is conflicting with this test

[cielf]

Checked it out functionally. Looks good. Have requested technical review from @dorner.

[cielf]

@dorner What I was trying to achieve with this one is to help the org_admins troubleshoot this themselves if they run into it -- hence the additional info about the possiblity of it being an inactive one.

All we're getting right now is "Something didn't work quite right -- try again?" -- so no specifics at all.

[dorner]

So would an acceptable solution just be to show the full error message, regardless of what that error is, rather than solving just for this specific issue?

[cielf]

Yes -- but I do want that subtlety about the inactive items in there if we can.

[Gabe-Torres]

Hi, I pushed something up that might work using i18n yaml file but I might be losing the thread here a bit. Do we want to display full errors or do we want to show a specific message for this scenario? Is there a way to do both?

I see we're adding errors on line 102 in the item controller and I also see on line 150 of the item model we are also adding errors. Is that how I should go about this issue?

[cielf]

Ideally, we want to show all the errors, including a specific message for this one. I'll leave specific commentary on how to @dorner.

[Gabe-Torres]

I think the i18n is overkill - we only support English in the app right now, it seems extremely odd to have just this one error localized. :D You can just throw that exact text when this situation happens, and display the full text of the error on the page.

I agree! Awesome I'll have this in by end of day! 🚀

[Gabe-Torres]

Hi! Throwing the exact text and displaying the full text of the error on the page made sense to me at the time. But then I got a bit confused. Apologies for dragging this one out y'all! @cielf mentioned she wanted the hint of an inactive item. From my understanding that isn't possible if I'm throwing the exact error message. I am checking that we are excluding that and throwing the exact error message for both conditions(blank parameters and name validation)

Or did you mean to insert whatever the message was about an inactive item and throw the full error message for other conditions in a conditional block? Which was the route we were going before I believe.

I sent up another pr that might be what we are looking for. It is similar to the i18n approach but instead, I was able to add the message for an already taken name within the validation in the Item model! It seems pretty snazzy and gives the user the desired message. Of course, this meant I had to change some other things.

1. Since the new flash error is throwing the whole error, I had to reflect that in the Item_system_spec on line 25.
2. There was a previous method in the Organization model, seed_items, which at some point has an error exception and looks for the presence of ActiveRecords previous default "been taken" message for an Item name that already exists. So I had it look for the new error message. No conflicts after those changes. All tests pass on my end after merging the most recent main.

Thank you for y'alls patience on this issue. I appreciate it!

[cielf]

Will do - probably tomorrow.

[cielf]

Grand. Merging it to main in a moment!