I wasn't sure where to put the new reporting_category field, so feel free to suggest any UI changes.

@coalest Please change the label "Reporting Category" to "NDBN Reporting Category" throughout. It's a bit redundant, but I think we should make it explicit. Once you've done that, please also update the user guide.

Other than that, looks great functionally!

@cielf Made the requested changes. Can you take a look at the documentation I wrote, to make sure it is accurate and makes sense?

Hey @coalest -- Yes, that makes sense. We need new screenshots, though, because the list/view/edit has changed.

@coalest -- I tried this with the production data, and I experienced an issue...
Slacking you with the screenshots (because I don't want to take the time to screen them for anything sensitive)

Further investigation reveals a data problem on prod -- some case that wasn't caught with the initial reporting category creation. Will investigate further. This will put a hold on merging this PR, but I don't see why we can't have @dorner do the technical review of this in the meantime.

To sum: Current state:

• Needs new screenshots for the user guide,
• Discovered a data problem on production that should halt the merge of this (investigating)
• Think it's fine for @dorner to go ahead with the technical review, though

Actually --- the problem is that items corresponding to kits don't have a reporting category (they aren't based on base items, and are allowed to have items from multiple reporting categories), but @coalest's code for displaying the items in the list assumes they do.

This raises the question of whether you should be allowed to put a reporting category on an item corresponding to a kit. I think we also need to disable that field for items that correspond to a kit (that "are" kits -- I think the relevant method is "housing_a_kit")

So, @dorner, this isn't ready for technical review after all.

@cielf Made the requested updates:

• Updated the screenshots in this issue after the text update.
• Fixed the error for items without a reporting category.
• Disabled the reporting category field when editing items housing a kit.

One question regarding UX. It seems like it would be a nice feature, such that when a user is creating a new item, when they select a base item, the reporting_category is automatically updated according to NDBN rules. It can still be changed, but at least it would save the user some time and reduce mistakes. What do you think? Should I add that?

Asking because right now the behavior is confusing. If I don't select any reporting category, it will be set automatically after the item is created. But if I select a reporting category (potentially the wrong one according to NDBN rules), it won't be changed on item creation. Is this expected?

Hrm. I'll need to double check / ask @dorner but I think this is the point where we can finally sever the connection to the base item. Which would solve the issue above, yes?

Not working on HE today, but should be able to look into it tomorrow.

@cielf I think severing the connection here between items and base items would be simple, but they are still linked in other places in the app so removing the connection entirely might be more involved?

Another thought, without the link with base items we wouldn't be able to recommend an NDBN reporting category for new items, right? Is that ok?

(Nods) The intent is that we will still have the items set up from the suite of base items when there is a new organization, but if they are entering a brand new item, they will have to indicate the NDBN reporting category themselves.

Hey @coalest -- Just checking in on this one! I think maybe it wasn't clear that we are good to go with severing the link to the base item.

Sorry for the late reply. Thanks for the clarification! I will work on removing the reliance on base items

@cielf When we say severing the connection between base items and items, does that mean I can remove the "Base Item" field from items in the UI?

From the show/edit pages:

It looks like that is the only place in the user/org_admin UI that mentions base item. So base items would only be found in the admin UI after this.

Also when you click "Export items" to get a csv on the items index page. That currently returns the name of the base item in the csv. Do we want to change that to reporting category? Or leave it as is?

Yup -- what we mean is that Item will no longer have a BaseItem.

The base items will still exist, but they are only going to be used to populate a new organization's items.

So...
Yes, it does mean that we can remove the base item from items in the UI.

I had forgotten about the base item in the export -- we will need to communicate to the users about that so they can adjust appropriately, but yep, remove it.

Ok just removed base items column from the items csv export. This should be ready for review again.

This is going to take me a couple of sessions to get through the testing of, I'm afraid
I ran into something when I was testing the new bank creation that looks unrelated, but I'm going to have to check with the current main and /or production code, etc., etc.

Edit: Confirmed that the thing I ran into is not a problem from this change. (Which I suspected all along)

So far so good, but I'll need another session to finish.

@dorner -- sorry, I neglected to indicate that I think this is ready for the first pass of a technical review -- the two things I've noted are quite minor changes (though they do need to be addressed before merging)

Ok @coalest -- We still need those two minor fixes, but once those are in we'll be good to go.

Hey @coalest -- are you able to finish this off (or should we get someone else to handle those last tweaks)? Thanks!

@dorner -- Could you take a quick look at my commits and make sure they're up to snuff before we merge this? Thanks.