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

Use Django storage for Wagtail deletion archive #8599

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

chosak
Copy link
Member

@chosak chosak commented Oct 2, 2024

These code changes propose using Django storage to store and serve the Wagtail deletion archive from #8310, instead of using Apache to serve those files.

This simplifies the configuration and makes it easier to test locally and easier to port in future to other webserver approaches.

With these changes, the deletion archive is now only available behind the Wagtail admin login, at the URL /admin/__deleted/. This URL provides a nicely formatted Wagtail report view where archives can be downloaded (instead of the current PR approach that uses Apache directory serving).

With this change, Django storage configuration has been migrated to settings.STORAGES from settings.STATICFILES_STORAGE, which was deprecated in Django 4.2 and removed in Django 5.1.

Additionally, with this change, cf.gov deployments that have not defined the WAGTAIL_DELETION_ARCHIVE_PATH environment variable still support importing archives; the archive-on-delete and archive download functionality is still disabled in those cases.

This commit also includes a minor bugfix to the archive import template which doesn't currently properly display the destination page title.

Screenshots

image

Notes and todos

TODO: Unit tests! I didn't invest time in making these pending @willbarton review of the general suggestion.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines
  • Future todos are captured in comments and/or tickets
  • Project documentation has been updated, potentially one or more of:
    • This repo’s docs (edit the files in the /docs folder) – for basic, close-to-the-code docs on working with this repo
    • CFGOV/platform wiki on GHE – for internal CFPB developer guidance
    • CFPB/hubcap wiki on GHE – for internal CFPB design and content guidance

These code changes propose using Django storage to store and serve
the Wagtail deletion archive from PR 8310 [0], instead of using Apache
to serve those files.

This simplifies the configuration and makes it easier to test locally
and easier to port in future to other webserver approaches.

With these changes, the deletion archive is now only available behind
the Wagtail admin login, at the URL /admin/__deleted/. This URL provides
a nicely formatted Wagtail report view where archives can be downloaded
(instead of the current PR approach that uses Apache directory serving).

With this change, Django storage configuration has been migrated to
settings.STORAGES from settings.STATICFILES_STORAGE, which was
deprecated in Django 4.2 and removed in Django 5.1.

Additionally, with this change, cf.gov deployments that have not defined
the WAGTAIL_DELETION_ARCHIVE_PATH environment variable still support
importing archives; the archive-on-delete and archive download
functionality is still disabled in those cases.

This commit also includes a minor bugfix to the archive import template
which doesn't currently properly display the destination page title.

[0] #8310
@chosak chosak requested a review from willbarton October 2, 2024 15:27
Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't invest time in making these pending @willbarton review of the general suggestion.

Please do! I love the general suggestion.

@chosak chosak force-pushed the wagtail-deletion-archive-storage branch from 6a1984a to 957a3fb Compare October 3, 2024 14:34
@chosak
Copy link
Member Author

chosak commented Oct 3, 2024

@willbarton thanks - I've added in tests that cover 100% of wagtail_deletion_archival (btw: could/should this be wagtail_deletion_archive?). The coverage failure here is due to existing missing coverage in the settings files which we unfortunately don't cover.

@chosak chosak marked this pull request as ready for review October 3, 2024 14:51
@willbarton
Copy link
Member

btw: could/should this be wagtail_deletion_archive?

I don't feel particularly strongly.

@chosak chosak merged commit 2b935c3 into experiment/bakery-export Oct 3, 2024
16 of 17 checks passed
@chosak chosak deleted the wagtail-deletion-archive-storage branch October 3, 2024 20:09
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.

2 participants