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

[WORLDSERVICE-464] Remove sw.js for Public Services #12566

Open
wants to merge 6 commits into
base: latest
Choose a base branch
from

Conversation

elvinasv
Copy link
Member

@elvinasv elvinasv commented Mar 25, 2025

Resolves JIRA: https://jira.dev.bbc.co.uk/browse/WORLDSERVICE-464

Summary

Removes sw.js for remaining Public Services

Code changes

  • Removed swPath from archive and scotland configs
  • Updated getWorldServices util to include scotland in the publicServices list
  • Updated getArticleSwRegex and getSwRegex to only use WS
  • Updated tests

Developer Checklist

  • UX
    • UX Criteria met (visual UX & screenreader UX)
  • Accessibility
    • Accessibility Acceptance Criteria met
    • Accessibility swarm completed
    • Component Health updated
    • P1 accessibility bugs resolved
    • P2/P3 accessibility bugs planned (if not resolved)
  • Security
    • Security issues addressed
    • Threat Model updated
  • Documentation
    • Docs updated (runbook, READMEs)
  • Testing
    • Feature tested on relevant environments
  • Comms
    • Relevant parties notified of changes

Testing

  • Manual Testing required?
    • Local (Ready-For-Test, Local)
    • Test (Ready-For-Test, Test)
    • Preview (Ready-For-Test, Preview)
    • Live (Ready-For-Test, Live)
  • Manual Testing complete?
    • Local
    • Test
    • Preview
    • Live

Additional Testing Steps

  • SW should not be accessible via the Public Service path (e.g., /news/sw.js).

Useful Links

@elvinasv elvinasv changed the title [WORLDSERVICE-464] Remove sw.js for Public Service [WORLDSERVICE-464] Remove sw.js for Public Services Mar 25, 2025
@elvinasv elvinasv marked this pull request as ready for review March 25, 2025 16:40
shouldMatchValidRoutes(validRoutes, articleSwPath);

const invalidRoutes = [
'/news/sw.js',
'/news/articles/sw.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering whether to add examples of all sw paths which are now invalid, for all Public Services? Especially if we have excluded them explictly from the regex.

So this would be:

  • /sport/articles/sw.js
  • /newsround/articles/sw.js
  • /archive/articles/sw.js
  • /scotland/articles/sw.js
  • /naidheachdan/sgeulachdan/sw.js

Copy link
Member Author

@elvinasv elvinasv Mar 26, 2025

Choose a reason for hiding this comment

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

Good point, updated

];

return services.filter(service => !publicServices.includes(service));
};

export const getArticleSwRegex = services => {
const serviceRegex = getServiceRegex(getWorldServices(services));
return `/:service(${serviceRegex})/:local(${articleLocalRegex})/sw.js`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the articleLocalRegex was for the public services, does it now make sense to remove this, and hardcode articles instead...?

Suggested change
return `/:service(${serviceRegex})/:local(${articleLocalRegex})/sw.js`;
return `/:service(${serviceRegex})/articles/sw.js`;

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Updated

Comment on lines 44 to 46
export const getArticleManifestRegex = services => {
const serviceRegex = getServiceRegex(getWorldServices(services));
return `/:service(${serviceRegex})/:local(${articleLocalRegex})/manifest.json`;
Copy link
Contributor

@karinathomasbbc karinathomasbbc Mar 26, 2025

Choose a reason for hiding this comment

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

This is potentially out of scope (or is it? - Let's get a second opinion!) Let's do this while we're here: per my comment above anything that uses getWorldServices no longer needs articleLocalRegex. If we did change it, we would also need to update some tests.

(Note that this we recently removed manifest paths from Public Services, and we just missed this.

Suggested change
export const getArticleManifestRegex = services => {
const serviceRegex = getServiceRegex(getWorldServices(services));
return `/:service(${serviceRegex})/:local(${articleLocalRegex})/manifest.json`;
export const getArticleManifestRegex = services => {
const serviceRegex = getServiceRegex(getWorldServices(services));
return `/:service(${serviceRegex})/articles/manifest.json`;

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. Updated

Copy link
Contributor

@Isabella-Mitchell Isabella-Mitchell 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 - one thing I noticed is we have some e2e tests which mention service worker in cypress/e2e/application/index.js. Will these be impacted? I'm taking a look to understand when we run these...

@karinathomasbbc
Copy link
Contributor

Looks good - one thing I noticed is we have some e2e tests which mention service worker in cypress/e2e/application/index.js. Will these be impacted? I'm taking a look to understand when we run these...

Ohhh that's a good shout, I had forgotten about that. I think we might not need to do anything, as these tests had already been removed for Public Services. I would have expected the Cypress tests to fail if we deleted something & didn't clean up the tests accordingly.

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.

4 participants