EES-6818 Add redirects for removed release pages#6644
EES-6818 Add redirects for removed release pages#6644Guy-HiveIT wants to merge 4 commits intodevfrom
Conversation
| }, | ||
| ]; | ||
| }, | ||
| async redirects() { |
There was a problem hiding this comment.
Hey @Guy-HiveIT this approach looks good to me! You may or may not be aware that we also have a getRedirectUrl function in server.js, so I don't know if at some point in the future if the 2 methods should be more consolidated into a single approach where it makes sense to do so?
There was a problem hiding this comment.
I wasn't aware of that! I don't know which ones will run first when a request comes in, but it probably does make sense to consolidate in one place. I think the server.js getRedirectUrl will have more flexibility but also looks less clear (and less standard, compared to the next recommended approach linked in the PR description).
I can try and move them to server.js now if you think that's best?
There was a problem hiding this comment.
I reckon it can be done outside of this PR, but thought I'd just make you aware :)
| Suite Teardown user closes the browser | ||
| Test Setup fail test fast if required | ||
|
|
||
| Force Tags GeneralPublic Local |
There was a problem hiding this comment.
I guess that this test doesn't have to be restricted only to local, or maybe I'm missing something :) But I think this can have Dev and probably PreProd and Prod too?
There was a problem hiding this comment.
I'm not sure so will defer to your knowledge - I copied those tags from another redirects robot test (redirects_www), but the general redirects does have the extra env tags on it, albeit with a comment:
#In local, public url contains authentication details. In order to make it work while running against dev- we need to get rid of it to compare public url against expected value.
I can see on the www redirects one, Sam changed it at some point to only GeneralPublic Local (EES-5160 Only run www tests against prod and local (#4948)) and it looks like there were issues on other environments with URL matching:
https://dfedigital.atlassian.net/browse/EES-5160?focusedCommentId=85834
I'm guessing we'd run into the same issues with this. Let me know what you think!
There was a problem hiding this comment.
I can't personally see a reason why we shouldn't run these tests against all envs nowadays. Happy that we give it a try and if we run into issues, we'll fix 'em!
And while we're at it, I've just tried running redirects_www.robot against Dev (by adding the Dev tag) and it also works fine, so I think we should reinstate those tests against the other envs as well!
There was a problem hiding this comment.
Great, have added those in now! Thanks
There was a problem hiding this comment.
I think redirects_www.robot is broken when running it on my machine against any environment that has basic auth. I believe get_www_url is breaking it as it's inserting www. before the basic auth details. So if PUBLIC_URL is:
https://user:[email protected]
after get_www_url it becomes
https://www.user:[email protected]
I would expect this to fail against all environments except Local, where we don't have basic auth details in PUBLIC_URL.
If we fixed that issue, I think also the URL comparison would still fail because of basic auth details being in PUBLIC_URL (i.e. https://user:pass@pre-production[...] != https://pre-production[...]
My inclination would be to leave redirects_www.robot alone and create a ticket to fix get_www_url so it can work even if basic auth details are included in the URL. To be honest, I'd even consider removing redirects_www.robot entirely as I don't think it provides much value - it seems it's only intention is to check that www.[ees] gets redirected to [ees].
There was a problem hiding this comment.
Ok maybe I'm wrong - seems others don't have the basic auth details inside their PUBLIC_URL - not sure if that changes relatively recently and/or where the basic auth user/pass are coming from when the UI tests are run.
There was a problem hiding this comment.
The www redirect doesn't exist on test or preprod (e.g. https://www.pre-production.explore-education-statistics.service.gov.uk/ goes to This site can't be reached) - so they should be removed from redirects_www.robot Force Tags - maybe add a comment saying why.
Apart from that I think it should all be good :)
There was a problem hiding this comment.
I've made the amend to not run www_redirects on Test or Preprod. As mentioned on slack, I don't see why it's not working on those envs, but we can add Dev at least and keep an eye out for failures.
There was a problem hiding this comment.
We could create a ticket to investigate it why the www redirect isn't working on Test and Preprod
The redirect doesn't seem to work for those environments
3 participants
When the redesigned release page goes live, there will be a couple of routes we are removing. We need to handle cases when users (or bots!) navigate to these pages. So I have added redirects from:
find-statistics/[publication]/data-guidancetofind-statistics/[publication]find-statistics/[publication]/[release]/data-guidancetofind-statistics/[publication]/[release]/explore#data-guidance-sectionfind-statistics/[publication]/prerelease-access-listtofind-statistics/[publication]find-statistics/[publication]/[release]/prerelease-access-listtofind-statistics/[publication]/[release]/help#pre-release-access-list-sectionI've added these redirects in the next js config file as per their docs.
Current behaviour if someone visits
publication/(data-guidance|prerelease-access-list)is to show the latest release's info for that page, but this isn't possible using the redirects within the next js config. Instead, these top level page redirects go to the publication itself, will then redirect to the latest release for that publication. I think this is an acceptable tradeoff, as it still takes the user to the correct release and I don't think these links are as useful as the release-specific ones.