-
Notifications
You must be signed in to change notification settings - Fork 5
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
EES 5633 - FE - Redirect the user to the latest version of a page if using an old Release slug in the URL #5419
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mmyoungman
reviewed
Nov 28, 2024
src/explore-education-statistics-frontend/src/middleware/pages/__tests__/redirectPages.test.ts
Outdated
Show resolved
Hide resolved
mmyoungman
reviewed
Nov 28, 2024
src/explore-education-statistics-frontend/src/middleware/pages/__tests__/redirectPages.test.ts
Show resolved
Hide resolved
jack-hive
changed the title
EES 5663 - FE - Redirect the user to the latest version of a page if using an old Release slug in the URL
EES 5633 - FE - Redirect the user to the latest version of a page if using an old Release slug in the URL
Nov 28, 2024
benoutram
reviewed
Dec 6, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it pretty tricky to review all of the different permutations that need to be tested but they seem to have all been covered and fall into the right categories! @jack-hive I'll just wait for you to review the additional comments I left before approving...
src/explore-education-statistics-frontend/src/middleware/pages/__tests__/redirectPages.test.ts
Outdated
Show resolved
Hide resolved
src/explore-education-statistics-frontend/src/middleware/pages/redirectPages.ts
Outdated
Show resolved
Hide resolved
src/explore-education-statistics-frontend/src/middleware/pages/__tests__/redirectPages.test.ts
Outdated
Show resolved
Hide resolved
jack-hive
force-pushed
the
EES-5633
branch
2 times, most recently
from
December 11, 2024 12:38
1277614
to
44a46be
Compare
jack-hive
changed the base branch from
dev
to
EES-5632-distinguishing-release-redirects-across-different-publications
December 11, 2024 14:52
benoutram
reviewed
Dec 12, 2024
src/explore-education-statistics-frontend/src/middleware/pages/redirectPages.ts
Outdated
Show resolved
Hide resolved
src/explore-education-statistics-frontend/src/middleware/pages/__tests__/redirectPages.test.ts
Outdated
Show resolved
Hide resolved
jack-hive
force-pushed
the
EES-5632-distinguishing-release-redirects-across-different-publications
branch
from
December 12, 2024 15:28
56cf3c6
to
eb5ac48
Compare
Base automatically changed from
EES-5632-distinguishing-release-redirects-across-different-publications
to
dev
December 12, 2024 15:49
benoutram
approved these changes
Dec 12, 2024
… and to allow us to define more accurately which routes we expect redirects for
…re we weren't isolating a module between test runs The module state (cache) of the `redirectPages` module was interfering with subsequent runs of the tests. So we have used Jest's built-in `isolateModulesAsync` method to ensure that the module state is not carried over between test runs.
…he redirect patterns + organising tests into groups
…different publications
… list of matched redirect URL Patterns
…the list of matched redirect URL Patterns
Was previously getting this error when running `pnpm run build` in the frontend project: ``` Failed to compile. ./..\..\node_modules\.pnpm\[email protected]\node_modules\lodash\lodash.js Dynamic Code Evaluation (e. g. 'eval', 'new Function', 'WebAssembly.compile') not allowed in Edge Runtime Learn More: https://nextjs.org/docs/messages/edge-dynamic-code-evaluation Import trace for requested module: ./..\..\node_modules\.pnpm\[email protected]\node_modules\lodash\lodash.js ./src\middleware\pages\redirectPages.ts ./src\middleware.ts ```
benoutram
approved these changes
Dec 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR consists of the frontend changes necessary to allow Release redirects to work.
Previously, we only had the ability to be able to redirect the user when we detected an old methodology/publication slug in the URL. But now, with the introduction of
ReleaseRedirects
in EES-5631 & EES-5632, we need to be able to handle redirects for releases too.During this work, I had to refactor how the redirects worked on the frontend. Previously, we just looked for a prefix of either
/methodology
or/find-statistics
in the URL and then checked if the next segment of the URL contained an old slug for either a methodology or publication, respectively, and then proceeded to redirect ANY child route.However, this no longer works with the introduction of release redirects, because the current pages which contain a release slug in their URL also contain a publication slug. For this reason, if we only checked for the occurrence of a prefix in the URL before redirecting the user on any child route, then the release slug wouldn't be taken into account if we found a redirect for the publication. Equally, if we introduce any further routes in the future which contain multiple possible redirect slugs in their URL, they will be impacted too.
For this reason, I have refactored the redirect logic to only check for very explicit patterns in the URL, when trying to determine whether or not the URL looks like one that can be redirected. I've split out the 'URL Patterns' into a list, which can be iterated through and easily added to. We use the URL Pattern Standard to implement the matching logic, and use Regex to check for the various valid alternatives for a pattern (including checking for trailing slashes, which should be allowed). I have intentionally only included existing routes that need to be checked for redirects, and have not included subsequent child-routes that don't exist yet. Any new routes in the future, that have the potential to be redirected, can easily be added to the list of URL Patterns we check against.
Every (current) possible URL Pattern has been tested via Jest unit tests.
Other Related Changes
redirectPages
module throughout each test. However, due to theredirectPages
module containing state in the form of the cached redirects (see thecachedRedirects
property), this was being carried over between tests and potentially interfering. This hasn't been detected until now, because the previous tests, although mocking the response from theredirectService.list
method, always returned the same mocked data from that method. So, the caching had no consequence to whether the test passed or failed. Now we have multiple test cases for each test, corresponding to each unique URL Pattern, it was necessary to use Jest'sisolateModulesAsync
to isolate specific modules for every test so that local module state doesn't conflict between tests (which was being flagged when doing theexpect(redirectService.list).toHaveBeenCalledTimes(1);
checks in the tests)