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.

Sign up for GitHub

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

Jump to bottom

feat: Restrict access to "offline" flows #3196

Merged
merged 6 commits into from
Jun 1, 2024
Merged

feat: Restrict access to "offline" flows #3196

merged 6 commits into from
Jun 1, 2024

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented May 24, 2024 •
edited
Loading

What does this PR do?

  • Adds check for flow.status to /published routes
    • If "offline", show an offline status page
  • Limits public role access to the analytics and analytics_logs table
    • Analytics logs cannot be written for "offline" flows
    • A few foreign key relationship were added to get this working
  • Updates E2E tests built on the assumption that flows are accessible once published

✅ Regression tests passing here https://github.com/theopensystemslab/planx-new/actions/runs/9288490634

image

@DafyddLlyr DafyddLlyr changed the base branch from main to dp/frontend-toggle-offline May 24, 2024 15:32
@github-actions GitHub Actions
Copy link

github-actions bot commented May 24, 2024 •
edited
Loading

🤖 Hasura Change Summary compared a subset of table metadata including permissions:

Updated Tables (2)

@DafyddLlyr DafyddLlyr force-pushed the dp/frontend-toggle-offline branch from c27ad55 to b2356a0 Compare May 24, 2024 15:33
@DafyddLlyr DafyddLlyr force-pushed the dp/hide-offline-flows branch 2 times, most recently from ceb2317 to f1aaee8 Compare May 24, 2024 15:41
@DafyddLlyr DafyddLlyr mentioned this pull request May 24, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented May 24, 2024 •
edited
Loading

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr marked this pull request as ready for review May 24, 2024 16:26
@DafyddLlyr DafyddLlyr requested a review from a team May 24, 2024 16:27
@DafyddLlyr DafyddLlyr marked this pull request as draft May 27, 2024 07:34
@DafyddLlyr DafyddLlyr removed the request for review from a team May 27, 2024 07:35
Base automatically changed from dp/frontend-toggle-offline to main May 28, 2024 15:15
@DafyddLlyr DafyddLlyr force-pushed the dp/hide-offline-flows branch 4 times, most recently from 05e665f to ff0119b Compare May 29, 2024 13:35
DafyddLlyr
DafyddLlyr commented May 29, 2024
View reviewed changes
editor.planx.uk/src/pages/OfflinePage.tsx Outdated
export const OfflinePage: React.FC = () => (
<StatusPage bannerHeading="Offline">
<Typography variant="body2">
This service is not currently available. Please check back later.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content here feels very minimal... open to suggestions!

Copy link
Member

Choose a reason for hiding this comment

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

Here's a maybe curveball question / suggestion (sorry!):

This service is not currently available to new applicants. Please check back later.

If you're resuming an application you previously started, please use the link sent to you via email.

Basically, what's our plan for handling active sessions for an "offline" service? Can we disable the /published route when no query params are present, but enable it for folks coming with query params email & sessionId ? Otherwise I fear we get into much murkier questions around extending session expiry periods if offline, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Also I can imagine a future enhancement to the editor form could be simple rich text input to control what your "Offline" page says (eg add a specific date if they choose - like after winter holidays). But default copy would be this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for copy suggestion, updated ✅

No need to apologise - it's good to raise and be explicit here about the behaviour. I agree that this is actually the smartest way forward, and best for users. I've made change which implements this, but as it needs to be at a "higher level" than Save and Return, I've opted to use a layout which wraps this view.

@DafyddLlyr DafyddLlyr requested a review from a team May 29, 2024 13:50
@DafyddLlyr DafyddLlyr marked this pull request as ready for review May 29, 2024 13:50
jessicamcinchak
jessicamcinchak reviewed May 30, 2024
View reviewed changes
editor.planx.uk/src/pages/OfflinePage.tsx Outdated
export const OfflinePage: React.FC = () => (
<StatusPage bannerHeading="Offline">
<Typography variant="body2">
This service is not currently available. Please check back later.
Copy link
Member

Choose a reason for hiding this comment

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

Here's a maybe curveball question / suggestion (sorry!):

This service is not currently available to new applicants. Please check back later.

If you're resuming an application you previously started, please use the link sent to you via email.

Basically, what's our plan for handling active sessions for an "offline" service? Can we disable the /published route when no query params are present, but enable it for folks coming with query params email & sessionId ? Otherwise I fear we get into much murkier questions around extending session expiry periods if offline, etc.

editor.planx.uk/src/routes/published.tsx Outdated Show resolved Hide resolved
@DafyddLlyr DafyddLlyr marked this pull request as draft May 30, 2024 12:40
@DafyddLlyr DafyddLlyr self-assigned this May 30, 2024
@DafyddLlyr DafyddLlyr force-pushed the dp/hide-offline-flows branch from f31edd1 to 97e6622 Compare May 31, 2024 13:01
@DafyddLlyr DafyddLlyr force-pushed the dp/hide-offline-flows branch from 97e6622 to 587de80 Compare May 31, 2024 13:07
@DafyddLlyr DafyddLlyr marked this pull request as ready for review May 31, 2024 13:09
@DafyddLlyr DafyddLlyr requested a review from a team May 31, 2024 13:09
@DafyddLlyr DafyddLlyr removed their assignment May 31, 2024
@DafyddLlyr
Copy link
Contributor Author

Toggle on/off to take a service live

jessicamcinchak
jessicamcinchak approved these changes Jun 1, 2024
View reviewed changes
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Thanks for latest updates here, works exactly as expected !

editor.planx.uk/src/pages/layout/OfflineLayout.tsx
return isFlowAccessible ? children : <OfflinePage />;
};

export default OfflineLayout;
Copy link
Member

Choose a reason for hiding this comment

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

Super clear solution ➕

@DafyddLlyr DafyddLlyr merged commit d9641ae into main Jun 1, 2024
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/hide-offline-flows branch June 1, 2024 08:10
RODO94 pushed a commit that referenced this pull request Jun 4, 2024
@DafyddLlyr @RODO94
feat: Restrict access to "offline" flows (#3196)
cf529f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessicamcinchak jessicamcinchak jessicamcinchak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DafyddLlyr @jessicamcinchak