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

[ENB-7652] Refactor flags and methods #3498

Open
wants to merge 19 commits into
base: stage
Choose a base branch
from

Conversation

swamu
Copy link
Contributor

@swamu swamu commented Jan 16, 2025

Copy link
Contributor

aem-code-sync bot commented Jan 16, 2025

Page Scores Audits Google
📱 /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.49%. Comparing base (6cb26a3) to head (ad9dfc5).

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #3498      +/-   ##
==========================================
+ Coverage   96.47%   96.49%   +0.02%     
==========================================
  Files         260      260              
  Lines       60582    60603      +21     
==========================================
+ Hits        58444    58481      +37     
+ Misses       2138     2122      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@swamu swamu force-pushed the ENB-7652-refactor-enablePerseV2 branch 2 times, most recently from fbe6994 to c835da7 Compare January 16, 2025 11:33
Copy link
Contributor

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

@swamu swamu force-pushed the ENB-7652-refactor-enablePerseV2 branch from c835da7 to f70653b Compare January 17, 2025 10:03
@swamu swamu requested review from Bhim12 and Nadeemakramn January 17, 2025 10:23
@swamu swamu force-pushed the ENB-7652-refactor-enablePerseV2 branch from 239ae41 to 96d7c65 Compare January 20, 2025 10:05
@swamu swamu force-pushed the ENB-7652-refactor-enablePerseV2 branch from 8e7e688 to e94c3e7 Compare January 21, 2025 12:06
@swamu swamu added the needs-verification PR requires E2E testing by a reviewer label Jan 23, 2025
@Nadeemakramn Nadeemakramn added verified PR has been E2E tested by a reviewer and removed needs-verification PR requires E2E testing by a reviewer labels Jan 23, 2025
@swamu swamu added the high priority Why is this a high priority? Blocker? Critical? Dependency? label Jan 23, 2025
@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Jan 23, 2025

Skipped merging 3498: [ENB-7652] Refactor flags and methods due to insufficient approvals. Required: 2 approvals

Copy link

@Nadeemakramn Nadeemakramn left a comment

Choose a reason for hiding this comment

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

Verified all scenarios in QA environment. Hence Approved it.

@swamu swamu requested review from sharg1 and akanshaa-18 January 23, 2025 11:50
libs/martech/helpers.js Outdated Show resolved Hide resolved
@vgoodric
Copy link
Contributor

@swamu I think it would be better to add the pzn v2 flag in config.mep and pull it back out of config when possible instead of passing it to so many functions. if you look at the top of the init function in personalization.js you'll see we add several settings for later use related to MEP. It feels like this will also make it easier to remove later if we fully roll the feature out.

Also wondering if we want to leverage the utils.js function getMepEnablement in enablePersonalizationV2. This would let us use parameter override to test the feature on pages before we have the file itself updated.

@swamu
Copy link
Contributor Author

swamu commented Jan 24, 2025

@vgoodric , noted and updated with both the points

@Nadeemakramn
Copy link

Retested all new commits, Good to Merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Why is this a high priority? Blocker? Critical? Dependency? Ready for Stage verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants