Skip to content

task/CDD 1379 page previews#3077

Open
jeanpierrefouche-ukhsa wants to merge 2 commits into
mainfrom
task/CDD-1379-page-previews
Open

task/CDD 1379 page previews#3077
jeanpierrefouche-ukhsa wants to merge 2 commits into
mainfrom
task/CDD-1379-page-previews

Conversation

@jeanpierrefouche-ukhsa
Copy link
Copy Markdown

@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa commented Mar 17, 2026

Description

CDD-1379 - Page Previews

Depends Upon: Front End branch: task/CDD-1379-page-previews (please pull the latest)

Date: 2026-02-27

Ticket: https://ukhsa.atlassian.net/browse/CDD-1379?search_id=055fe61d-bee9-48d9-80bc-ffb0f1c26b76&referrer=quick-find

Authors: Jean-Pierre Fouche

Impact: Affects all pages - broad testing required

Testing: Comprehensive unit tests supplied. UAT needed.

Summary

Allow editors of headless composite pages to click a Preview button that immediately redirects them to the external frontend application, rather than opening the built-in Wagtail iframe preview. Preview URLs include a short-lived signed token so the frontend can safely fetch draft content from the CMS.

Additionally, allow users to select an embargo date in order to preview otherwise embargoed data.

Continued in ./changelog/2026-02-27/CDD-1379.page-previews.md

Fixes #CDD-1379


Type of change

Please select the options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Tech debt item (this is focused solely on addressing any relevant technical debt)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests at the right levels to prove my change is effective
  • I have added screenshots or screen grabs where appropriate
  • I have added docstrings in the correct style (google)

Comment thread cms/common/models.py
Comment thread cms/dashboard/models.py
Comment thread cms/dashboard/models.py
Comment thread cms/dashboard/serializers.py
Comment thread cms/dashboard/views.py
Comment thread cms/dashboard/views.py Outdated
Comment thread cms/dashboard/viewsets.py
Comment thread cms/dashboard/viewsets.py Outdated
Comment thread cms/dashboard/viewsets.py
Comment thread cms/dashboard/viewsets.py
Comment thread cms/dashboard/wagtail_hooks.py
Comment thread cms/dashboard/wagtail_hooks.py Outdated
Comment thread cms/dashboard/wagtail_hooks.py
Comment thread cms/dashboard/wagtail_hooks.py Outdated
Comment thread metrics/api/settings/default.py Outdated
Comment thread metrics/api/settings/local.py
Comment thread scripts/_quality.sh
Comment thread tests/unit/cms/dashboard/test_serializers.py Outdated
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch 6 times, most recently from 46d2401 to 23231b1 Compare March 17, 2026 21:34
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa marked this pull request as ready for review March 18, 2026 09:42
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa requested a review from a team as a code owner March 18, 2026 09:42
@jrdh jrdh force-pushed the task/CDD-1379-page-previews branch from 0ee49d3 to 258dd9a Compare March 26, 2026 16:25
Copy link
Copy Markdown
Contributor

@jrdh jrdh left a comment

Choose a reason for hiding this comment

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

I've made lots of comments but this is looking really good! Worked nicely on my local machine 🙂

Comment thread cms/dashboard/views.py Outdated
Comment thread changelog/2026-02-27/CDD-1379.page-previews.md Outdated
Comment thread cms/dashboard/views.py Outdated
Comment thread cms/dashboard/views.py Outdated
Comment thread cms/dashboard/views.py Outdated
Comment thread scripts/_quality.sh Outdated
Comment thread scripts/_tests.sh
Comment thread tests/unit/cms/dashboard/test_preview_views_and_hooks.py Outdated
Comment thread tests/unit/cms/dashboard/test_preview_views_and_hooks.py
Comment thread tests/unit/cms/dashboard/test_preview_views_and_hooks.py Outdated
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch from 258dd9a to 3a013f1 Compare March 30, 2026 13:33
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch 6 times, most recently from aacf010 to 93f696d Compare April 29, 2026 14:11
@sonarqubecloud
Copy link
Copy Markdown

@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch from 93f696d to fefbfc5 Compare May 13, 2026 10:48
Comment thread cms/dashboard/management/commands/build_cms_site.py Outdated
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch 4 times, most recently from e15a8fe to 06661aa Compare May 13, 2026 13:48
Copy link
Copy Markdown
Contributor

@jrdh jrdh left a comment

Choose a reason for hiding this comment

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

Still looking good. Made a few comments in line but nothing major there. I have concern which I think we have to fix and a question.

The concern is that I think the /nocache/ functionality should be protected by the same method we're using for the /preview/ URLs (i.e. signing) as without any authentication on them they allow access to potentially unembargoed data and open up endpoints malicious users could DDOS us with due to the uncached nature of the responses.

The question is whether you considered using the Django request context instead of the ContextVars? I don't have a strong feeling either way but wasn't sure if there was a best practice method Django recommended for variables that live for the length of a request and need to be passed around to different functions? The only issues I could foresee is if Django actually uses multiple threads to respond to a request and doesn't copy the context over which would mean the embargo date wouldn't always be set correctly. I think this is really unlikely but just wanted to raise it as a concern related to what Django best practice is.

As an aside, I would have much preferred if this had been worked on in a series of commits rather than one fat one. It's not good for future maintainability/the git history, and it makes it much harder to review as I can't just look at the new commits created after my last review.

Comment thread common/virtual_clock.py Outdated
Comment thread common/virtual_clock.py Outdated
Comment thread common/virtual_clock.py Outdated
Comment thread common/virtual_clock.py Outdated
Comment thread common/virtual_clock.py Outdated
Comment thread metrics/api/settings/default.py
Comment thread validation/shared.py Outdated
@jeanpierrefouche-ukhsa
Copy link
Copy Markdown
Author

The question is whether you considered using the Django request context instead of the ContextVars? I don't have a strong feeling either way but wasn't sure if there was a best practice method Django recommended for variables that live for the length of a request and need to be passed around to different functions? The only issues I could foresee is if Django actually uses multiple threads to respond to a request and doesn't copy the context over which would mean the embargo date wouldn't always be set correctly. I think this is really unlikely but just wanted to raise it as a concern related to what Django best practice is.

Hi Josh,

Thanks for your comment.

Yes, Django request context was considered and I appreciate that it is often the go-to approach in Django. However, Django request context needs to be passed around. This would break all of the code interfaces (signatures) and make the application more complicated. ContextVars allow the code to pull in request-scoped state, without passing state around. A few points about ContextVars:

  • they are safe (won't leak) within the execution context (which is deeper than just the thread. See Python Docs on ContextVar, the section on asyncio support
  • if spawning a new thread, the onus is now placed upon the thread creator to use ContextVar.copy_context. Note that this is an edge case, in which the thread creator needs access to embargo_time. In my view, this is cleaner, relatively simpler, and more reliable than passing Django request state around explicitly. Anybody spawning a new thread should, in any case, reasonably assume that there are going to be issues with accessing state and take appropriate steps to ensure access.

Example code, below:

import threading
import contextvars

embargo_date = contextvars.ContextVar("embargo_date")

def worker():
    # ContextVar is available here (current execution context, current thread)
    print(embargo_date.get())


def start_thread():
    embargo_date.set("2026-05-14")

    # Capture current context
    ctx = contextvars.copy_context() # <================== use copy_context if you need the embargo_date

    # Run worker inside copied context
    thread = threading.Thread(target=lambda: ctx.run(worker))  # <===== run a thread within the context
    thread.start()
    thread.join()
    ```

@jeanpierrefouche-ukhsa
Copy link
Copy Markdown
Author

The concern is that I think the /nocache/ functionality should be protected by the same method we're using for the /preview/ URLs (i.e. signing) as without any authentication on them they allow access to potentially unembargoed data and open up endpoints malicious users could DDOS us with due to the uncached nature of the responses.

Hi Josh,

I have reworked the flow so that routes to "View Live" are now authenticated.
Please see the changes for your review.

Many Thanks!

@jrdh

@jeanpierrefouche-ukhsa
Copy link
Copy Markdown
Author

As an aside, I would have much preferred if this had been worked on in a series of commits rather than one fat one. It's not good for future maintainability/the git history, and it makes it much harder to review as I can't just look at the new commits created after my last review.

Hi Josh,

Yes, I can see where you are coming from! In a large commit like this it must have been less than helpful having to do the second review, not having a baseline to review it against.

@jrdh

@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch 4 times, most recently from 37b3b50 to 8a7d589 Compare May 19, 2026 20:07
  Enable page previews to the configured frontend URL
    - with set embargo date
    - support for per-request disabling of caching
      via Cache-Control headers from the client
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch 4 times, most recently from 5f25667 to ec212db Compare May 19, 2026 21:16
PR Review #3 - Apply auth to View Live

- move shared.py to appropriate locations
- django UTC timezone consistency in virtual_clock.py
- remove redundant embargo context helper
- tidy up typing import as it is used only once (from typing as t)
- default to None if page previews not enabled
- fix keyword args issue in virtual_clock.py - remove positional arg

task/CDD-1379-page-previews
update idna to 3.15
@jeanpierrefouche-ukhsa jeanpierrefouche-ukhsa force-pushed the task/CDD-1379-page-previews branch from ec212db to 2eb2ddb Compare May 19, 2026 21:19
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@jrdh jrdh left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for making those changes JP 🎉

Couple of minor comments in line and a question:

  • If I remove the page_id or change it to a different valid value (e.g. the ID of another page) from either /preview or /nocache URLs it still works fine - is this intended? If so do we need that query parameter?

Otherwise, this is good to go to QA as far as I'm concerned!

Comment thread cms/dashboard/views.py
raise PermissionDenied

embargo_time_value = request.GET.get("embargo_time")
route = request.GET.get("route", "nocache")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any particular functional reason to default to nocache here instead of error if the route parameter isn't set?


We register an admin redirect endpoint
(`/admin/preview-to-frontend/<pk>/`) that signs a short-lived
(`/admin/redirect-to-frontend/<pk>/`) that signs a short-lived
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extremely minor note, because we expose /admin as /cms-admin, the URL path starts cms-admin not admin.

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