Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "destructive approval" from our approval step for existing providers #530

Merged
merged 6 commits into from
Nov 22, 2023

Conversation

mrchrisadams
Copy link
Member

@mrchrisadams mrchrisadams commented Oct 20, 2023

This PR changes the behaviour to remove the default of deleting some data about a provider when we try to merge a new verification request into an existing provider.

Previously we had some logic like this:

if self.provider:
hp = Hostingprovider.objects.get(pk=self.provider.id)

# delete related objects, they will be recreated with recent data
hp.services.clear()

for asn in hp.greencheckasn_set.all():
    asn.delete()

for ip_range in hp.greencheckip_set.all():
    ip_range.delete()

for doc in hp.supporting_documents.all():
    doc.delete()

We want to preserve existing IPs and documents where possible, so this PR moves to archive these safely instead.

What we do instead

With the new logic, we now have a set of archive and unarchive methods on the objects we want to preserve, and where necessary, we use a new model manager to avoid surfacing archived content. This reduces the risk of data loss.

With IP Ranges and ASN

As mentioned before, in the approval process, we call archive() on any matching IPs or ASNs. This uses the active state, so they no longer are used in greenchecks. If a verification request still has a matching IP range or ASN, we unarchive them, so they are visible once again and show in greenchecks.

With uploaded supporting evidence

Supporting evidence is a little more complicated. I've introduced an archived property on the AbstractSupportingDocument model that all our evidence types inherit from.

During the approval process, we call archive() on all supporting evidence on a provider, and then if there is matching information in the verification request, we call unarchive() again, making it visible to the hosting provider. We honour the original public status, so information that was uploaded as not public, stays as not public.

We do this by using a different model Manager on the HostingProviderSupportingDocument model, which means all queries by default exclude archived information. See NonArchivedEvidenceManager for more.

If we need to access the archived evidence we still have an model Manager which does offer unfiltered queries - we can use this to search for archived content if need be.

class HostingProviderSupportingDocument(AbstractSupportingDocument):
    """
    The subclass for hosting providers.
    """

    # our default manager should filter out archived items
    objects = NonArchivedEvidenceManager()

    # but we still should have access if neeed by via the
    # original non-filtered manager
    objects_all = models.Manager()

@mrchrisadams mrchrisadams temporarily deployed to staging October 25, 2023 16:01 — with GitHub Actions Inactive
@mrchrisadams
Copy link
Member Author

OK, we have some new logic now for this to avoid deleting existing IP ranges, ASNs and supporting evidence.

I want to document the approach being taken here, and see how it works on staging before we merge into prod, as I ended up introducing a new 'archived' property on uploaded evidence, that we check for.

@mrchrisadams
Copy link
Member Author

@mrchrisadams mrchrisadams changed the title WIP - Remove "destructive approval" from our approval step for existing providers Remove "destructive approval" from our approval step for existing providers Nov 9, 2023
@mrchrisadams mrchrisadams merged commit e7b0b95 into master Nov 22, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant