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

[NASAA-271] Google Discover/Top stories experiment cleanup #12218

Draft
wants to merge 8 commits into
base: latest
Choose a base branch
from

Conversation

hotinglok
Copy link
Collaborator

@hotinglok hotinglok commented Nov 28, 2024

Resolves NASAA-271

Overall changes

Removes remaining code specific to the Google Discover/Top Stories experiment. Keeps changes to various analytics functions and the <AmpExperiment/> component as these can be used for future AMP experiments.

Code changes

  • Removes /experimentTopStories from /ArticlePage.
  • Updates AMP experiment properties in pageViewBeaconValues in /atiUrl. These are still optional properties that won't be sent unless ampExperimentName is present, so they won't affect regular usage. Keeping as it will be used again in future AMP experiments.
  • Adds readme to <AmpExperiment/>. This component should remain as it can be used for future AMP experiments. It is a generic component that accepts an experimentConfig and an analyticsConfig. It should only be added to pages when an AMP experiment will be ran.

Testing

Most likely no manual testing is required as regression testing of News/Sport article pages will have been done in the ticket prior to this (#12219).

Ensure all unit/e2e tests pass.

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

@hotinglok hotinglok changed the title [NASAA-271] Top stories experiment cleanup [NASAA-271] Google Discover/Top stories experiment cleanup Nov 28, 2024
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed the experiment-specific styling added to topStoriesSection here, but this was originally combined with featuresSection as they have the same styles (commit showing this).
Should I combine these together again or keep them separate? We don't have any plans at the moment with either section, but just curious if there was any preference to keep these separate. Happy either way.

value: `${experimentVariant}`,
value: `VARIANT(${ampExperimentName})`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed this to not depend on experimentVariant so that it won't clash with any other experiments running that might need this property as well. Should have done this in the first place!

Comment on lines -228 to 224
key: 'mv_experiment_id',
description: 'AMP experiment name',
value: `${ampExperimentName}`,
wrap: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ended up being somewhat redundant when we looked at the data. We rarely filtered using this as a column in the Piano dashboard and the mv_test property (shows as MVT - Test in the dashboard properties) is shown by default in the dashboard.

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.

1 participant