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

removed references to 'user-features' module booting commercial bu… #1130

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dskamiotis
Copy link
Contributor

@dskamiotis dskamiotis commented Nov 2, 2023

What does this change?

This pull request removes all references to the 'user-features' module from the project. The 'user-features' module was previously used for managing user-related data and features but is no longer needed in this codebase.

Important Note
Some functions from 'user-features' are still required for specific features like adblock-ask.ts and commercial-features.ts, which rely on functions like shouldHideSupportMessaging() and isAdFreeUser(). To accommodate this, a partial user-features.ts file has been retained for these functions.

This may not be a desirable long term solution but allows us to successfully test the user-features being set in one place (dotcom-rendering)
A discussion could be had regarding a centralised place for these functions eg. https://github.com/guardian/csnx/tree/main/libs/%40guardian

Why?

The 'user-features' module is no longer needed in this codebase as its functionality has been ported over to the dotcom-rendering repository. This change prevents redundant setting and retrieval of cookies, as user state management is now handled by 'dotcom-rendering'.

This revised version maintains the essential information while being somewhat more concise and to the point. It still explains the purpose and the reason for the change.

Testing

Cookies in browser
Screenshot 2023-11-03 at 09 52 51

@dskamiotis dskamiotis requested a review from a team as a code owner November 2, 2023 09:20
Copy link

changeset-bot bot commented Nov 2, 2023

🦋 Changeset detected

Latest commit: dac5397

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@guardian/commercial Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dskamiotis dskamiotis force-pushed the ds/remove-user-features-from-commercial branch from eab8db3 to a7ee269 Compare November 2, 2023 09:24
Copy link
Contributor

@domlander domlander left a comment

Choose a reason for hiding this comment

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

Nice work! My comments are mostly other things that look like they can be deleted

src/lib/user-features.ts Show resolved Hide resolved
src/commercial.consentless.ts Show resolved Hide resolved
src/standalone.commercial.ts Outdated Show resolved Hide resolved
src/lib/experiments/ab.spec.ts Outdated Show resolved Hide resolved
src/lib/user-features.ts Show resolved Hide resolved
src/lib/user-features.spec.ts Show resolved Hide resolved
@dskamiotis dskamiotis force-pushed the ds/remove-user-features-from-commercial branch from b60bd32 to 661c5d4 Compare November 2, 2023 15:34
@dskamiotis dskamiotis force-pushed the ds/remove-user-features-from-commercial branch from 661c5d4 to dc1d33f Compare November 2, 2023 15:46
@dskamiotis dskamiotis added the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Nov 2, 2023
@dskamiotis dskamiotis added [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR and removed [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR labels Nov 2, 2023
Copy link
Contributor

github-actions bot commented Nov 2, 2023

🚀 0.0.0-beta-20231102170518 published to npm as a beta release

@Jakeii
Copy link
Member

Jakeii commented Dec 8, 2023

This PR still needed @dskamiotis?

@domlander
Copy link
Contributor

This PR still needed @dskamiotis?

I believe it is, but will need a few changes, i.e deleting user-features, and a rebase on main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants