Fix ProviderTagMapping removal leaving Classifications

[agrare]

A read-only Classification does not mean that it should not be deleted. Classifications that are seeded from fixtures aren't read_only which would be the only reason why we would want to prevent read_only deletion.

When you try to delete a Tag Mapping you get an error:

And the Classification and Tag are still present:

vmdb(dev)> Classification.find(196)
=> #<Classification:0x00007f05fa7103c0 id: 196, description: "amazon:vm|company", icon: nil, read_only: true, syntax: "string", single_value: true, example_text: nil, tag_id: 198, parent_id: nil, show: true, default: nil, perf_by_tag: nil>vmdb(dev)> Classification.find(196).tag
=> #<Tag:0x00007f05fa5a1a48 id: 198, name: "/managed/amazon:vm:company"> 

Related #22232

[agrare]

So while we don't want to prevent deletion at the model level, we do want to prevent the user from deleting or editing these via the UI/API.

The API has the following logic: https://github.com/ManageIQ/manageiq-api/blob/master/app/controllers/api/categories_controller.rb#L7-L16
And I don't believe you can view provider tag categories in the UI (we filter them out).

So while the API could be replaced with supports_not :delete this PR doesn't open the door to letting the user delete these.

[Fryguy]

ReadOnlyMixin does want to prevent deletion at the model level, which is why it has a special bypass for seeding. That is, read_only generally means "seeded", and so that bypass allows deletion by the seeding code, but nothing else. In the case of Classifications though, I'm not sure it should, specifically because we have this whole mapped classification concept. So, I'm not sure if we should remove it here - I could be convinced though - need to think it through.

I think part of the problem is that read_only was "reused" by the Provider Tag Mapping code, and possibly shouldn't have been. I'm not sure though what the right answer is - probably a "provider_owned" flag in addition to "read_only", and then some logic when both values (or one or the other) is set.

[agrare]

ReadOnlyMixin does want to prevent deletion at the model level, which is why it has a special bypass for seeding. That is, read_only generally means "seeded", and so that bypass allows deletion by the seeding code, but nothing else.

Right that is why I dropped ReadOnlyMixin from this class as opposed to changing ReadOnlyMixin itself.

Also the seeded Classifications aren't read_only, https://github.com/ManageIQ/manageiq/blob/master/db/fixtures/classifications.yml#L5 so we aren't preventing seeded classifications from being deleted today as it is.

I think part of the problem is that read_only was "reused" by the Provider Tag Mapping code, and possibly shouldn't have been.

If Classification#read_only means don't allow the user to edit/delete it, then I think it is used correctly since it doesn't appear to mean seeded at least in this case. That does appear to be inconsistent with the other models, but for this case specifically I think preventing delete of read_only classifications isn't correct

[Fryguy]

Also the seeded Classifications aren't read_only, https://github.com/ManageIQ/manageiq/blob/master/db/fixtures/classifications.yml#L5 so we aren't preventing seeded classifications from being deleted today as it is.

Right I'm saying they should have been (more bugs). Then again, users might want to add tags to the categories, or perhaps delete seeded ones (which doesn't work).

If Classification#read_only means don't allow the user to edit/delete it, then I think it is used correctly since it doesn't appear to mean seeded at least in this case. That does appear to be inconsistent with the other models, but for this case specifically I think preventing delete of read_only classifications isn't correct

Sorry I didn't word my response correctly. read_only was correctly used if we go by the name of the column - we didn't want these deleted, so we set that field to true (no faults laid on the author that used it). However, traditionally read_only actually meant "seeded" (I've wanted to rename these columns forever but it's low on my priority list). So in that sense using the column meant for "seeded" records may not have been the correct choice, but I admit there's no way for people to know that based on that name. It's a mess :( .