[Task]: Handle orphaned cinder policies on AMO

[wagnerand]

Description

When cinder policies are synced from cinder to AMO (triggered in the AMO admin tools), it can happen that one or more cinder policies are no longer on cinder (or rather no longer included in the sync). Their AMO counterpart becomes orphaned.

If the AMO instance of the cinder policy is not linked to any review reasons, it is deleted. This is fine.

If the AMO instance of the cinder policy is linked to a review reason, nothing happens. This can create issues, as these cannot be deleted even if we wanted to, and more-so it's not visible to admins which ones are orphaned.

To resolve the visibility issue, any orphaned CinderPolicy should be marked as such during the sync. This could be as easy as prepending the name with !ORPHANED! or similar, or we might take a more structured approach and introduce a dedicated field on the model. ~See below why.~

~To close this gap, we should allow any* CinderPolicy instance to be deleted if it does not have a ReviewReason attached. Attaching and detaching is a manual process, so any existing and used CinderPolicies would have to be detached by an admin first. Any new CinderPolicy that was just synced and is not in use yet could technically be deleted as well, and that is fine. It is not used and it if still exists in the next sync, it would be added again.~

~*"any" is really a shortcut to ease implementation. If we want to make this more precise and more strict, we can do so, which would require e.g. an orphaned flag that gets set on a CinderPolicy instance during sync. With that, we could allow deletion only for instances that both have that flag set and no ReviewReason attached.~

~@eviljeff I would like to get your thoughts on this.~

After talking to @eviljeff we aligned on the following approach.

An orphaned CinderPolicy is a CinderPolicy whose uuid is not in the response of the request triggered by the "Sync" action.

During sync, an orphaned CinderPolicy should be automatically deleted if it does not meet any of the following criteria should be deleted:

• CinderPolicy is linked to a ReviewActionReason (i.e. ReviewActionReason.cinder_policy)
• CinderPolicy was used in a CinderDecision (i.e. CinderDecision.policies)
• CinderPolicy is directly exposed in the reviewer tools (i.e. CinderPolicy.expose_in_reviewer_tools)

If any of the criteria are met for an orphaned CinderPolicy during the sync, the CinderPolicy should be marked as orphaned, as per the above.

Acceptance Criteria

  ### Milestones/checkpoints
  - [ ] During sync, an orphaned `CinderPolicy` should be automatically deleted if it does **not meet any** of the criteria defined in the description.
  - [ ] If any of the criteria **are** met for an orphaned `CinderPolicy` during the sync, the `CinderPolicy` should be marked as orphaned.

Checks

• [X] If the issue is ready to work on, I have removed the "needs:info" label.
• [X] If I have identified that the work is specific to a repository, I have removed "repository:addons-server" or "repository:addons-frontend"

┆Issue is synchronized with this Jira Task

[eviljeff]

I think we can have some kind of label/status that indicates they aren't an active policy on Cinder, but I'd prefer that the flow be more like, change made to ReviewActionReason because the CinderPolicy is now obsolete, and then press the sync button again to clear it up, than enabling manual deletes.

To add some clarity to some of the details in the description:

When cinder policies are synced from cinder to AMO (triggered in the AMO admin tools), it can happen that one or more cinder policies are no longer on cinder.

I couldn't find any way to delete policies in cinder using their UI. (Once published, they can only have their status changed to archived). They can lose the AMO label though which, as we only care about AMO policies, is effectively the same thing.

If the AMO instance of the cinder policy is not linked to any review reasons, it is deleted. This is fine. If the AMO instance of the cinder policy is linked to a review reason, nothing happens. This can create issues, as these cannot be deleted even if we wanted to, and more-so it's not visible to admins which ones are orphaned.

The policy instances are also not deleted if there are one or more decisions that rely on them, or we'd lose the data. We should probably expose that count, at least, to make it clearer why something is sticking around even after it's obsolete.

[wagnerand]

@eviljeff I updated the description after our conversation, could you check whether it accurarately represents what we talked about and we you'd expect? Thanks.

[diox]

There will be a new present_in_cinder field, populated when syncing, and visible in the admin. Policies in use but not/no longer in Cinder after sync will have that field set to True. Policies not in use but not/no longer in Cinder after sync will be deleted.

[ioanarusiczki]

I tried a new sync on dev.

From 87 policies now there's 96.

I see 2 with "X" at Present in Cinder, I'm not sure these should have been deleted?

https://addons-internal.dev.mozaws.net/en-US/admin/models/abuse/cinderpolicy/29/change/ https://addons-internal.dev.mozaws.net/en-US/admin/models/abuse/cinderpolicy/37/change/

Another thing I've noticed on stage is that there's lots of policies exposed in rev tools (for example all acceptable use policies or consent, content, security, privacy policy and so on) admin sort of shows an "x" next to most of these policies at "Expose in reviewer tools"

[wagnerand]

Another thing I've noticed on stage is that there's lots of policies exposed in rev tools (for example all acceptable use policies or consent, content, security, privacy policy and so on) admin sort of shows an "x" next to most of these policies at "Expose in reviewer tools"

A few months ago I went through the effort and synchronized review reasons on prod with stage. At that point, the switch to expose a review reason in the reviewer tools was not implemented yet afair, therefore, they are mostly disabled. We should probably enable them. It would also be nice if we could keep the cinder policies and reasons largely in sync with prod. This would help with spotting errors only and help onboarding reviewers. Abhi or I can take care of the maintenance, but it would mean that we try and avoid changes. @ioanarusiczki would that work for you?

[diox]

So this is annoying to see because we don't expose these relations from the Cinder Policy but:

The first one is attached to this review action reason: https://addons-internal.dev.mozaws.net/en-US/admin/models/reviewers/reviewactionreason/36/change/

The second one is attached to multiple ContentDecision such as this one: https://addons-internal.dev.mozaws.net/en-US/admin/models/abuse/contentdecision/4291/change/

So both have some history attached to them and that's why they were kept.

[ioanarusiczki]

@wagnerand

I remember about you setting up the policies for stage. Since then I have not been doing any changes - syncs or adding/removing policies for the rev tools "reason" section. And it's fine by me. AMO Dev continues to have a different setup - I assume here's where I could still make changes?

@diox I understand now, so if it's been used already then it's not going to be deleted.

[wagnerand]

@ioanarusiczki yes, we can make any changes we want on -dev. If we want to make changes on stage, we could coordinate on that, and if it turns out to be cumbersome, we could revisit this soft-freeze of data.

[ioanarusiczki]

@wagnerand Got it.