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

MWPW-146209 [MILO][MEP] MEP param override does not work if manifest sent by Target and pzn and chosen column not in Target #2119

Merged
merged 14 commits into from
Apr 10, 2024

Conversation

vgoodric
Copy link
Contributor

@vgoodric vgoodric commented Apr 8, 2024

Steps to reproduce:

  1. Create a manifest
  2. Create a page that references that manifest in its personalization metadata
  3. Create a Target activity that serves the same manifest to the page
  4. Update the manifest to have a new column but don't update the Target offer
  5. Try to use the MEP button to spoof the new column

Solution: don't check if the selected variant should be overridden until the manifests have been consolidated in cleanAndSortManifestList function

Example page setup that is sending an old version of manifest via Target and is trying to spoof a new column not sent by Target:

Resolves: MWPW-145303

Test URLs:

Also included a tiny CSS fix for "instant" links

@vgoodric vgoodric requested a review from a team as a code owner April 8, 2024 19:51
Copy link
Contributor

aem-code-sync bot commented Apr 8, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link
Contributor

aem-code-sync bot commented Apr 8, 2024

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

@vgoodric vgoodric added the trivial PR doesn't require E2E testing by a reviewer label Apr 8, 2024
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.40%. Comparing base (1f48cda) to head (5ef08d6).

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #2119      +/-   ##
==========================================
+ Coverage   96.35%   96.40%   +0.05%     
==========================================
  Files         165      165              
  Lines       43534    43543       +9     
==========================================
+ Hits        41947    41979      +32     
+ Misses       1587     1564      -23     

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

@vgoodric vgoodric added bug Something isn't working run-nala Run Nala Test Automation against PR labels Apr 8, 2024
@vgoodric vgoodric changed the title Mepparamtarget MWPW-146209 [MILO][MEP] MEP param override does not work if manifest sent by Target and pzn and chosen column not in Target Apr 8, 2024
@vgoodric vgoodric removed the run-nala Run Nala Test Automation against PR label Apr 9, 2024
@afmicka
Copy link
Contributor

afmicka commented Apr 9, 2024

@vgoodric these changes are breaking mep preview functionality and our promotions nala tests, thats why it was not passing. We have added those tests recently so please do not skip run-nala label.

If you go to the url below and try to set to default any of the manifests and preview, it will not go through. It reloads again with all the manifests set to all. Could you please review?
https://mepparamtarget--milo--adobecom.hlx.live/drafts/nala/features/promotions/promo-default
Screenshot 2024-04-09 at 09 40 12 Screenshot 2024-04-09 at 09 40 20

It works on: https://main--milo--adobecom.hlx.live/drafts/nala/features/promotions/promo-default

cc: @nateekar @3ch023 @Roycethan

Copy link
Contributor

@afmicka afmicka left a comment

Choose a reason for hiding this comment

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

Please review MEP preview functionality on existing pages with multiple manifests like UK acom homepage as well. The changes are breaking it there too.

https://main--homepage--adobecom.hlx.page/uk/homepage/index-loggedout?milolibs=mepparamtarget

@vgoodric vgoodric added the run-nala Run Nala Test Automation against PR label Apr 9, 2024
@vgoodric
Copy link
Contributor Author

vgoodric commented Apr 9, 2024

@afmicka I turned off nala because there was something broken on stage as well. So it wasn't instructive. But I think everything is working now. Can you confirm?

libs/features/personalization/personalization.js Outdated Show resolved Hide resolved
@afmicka
Copy link
Contributor

afmicka commented Apr 10, 2024

@afmicka I turned off nala because there was something broken on stage as well. So it wasn't instructive. But I think everything is working now. Can you confirm?

Thanks @vgoodric. Confirming it is ok now.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Blainegunn
Copy link
Contributor

@vgoodric no verified label. Please add

@vgoodric vgoodric added the verified PR has been E2E tested by a reviewer label Apr 10, 2024
@Blainegunn Blainegunn merged commit b8cd0bf into stage Apr 10, 2024
16 checks passed
@Blainegunn Blainegunn deleted the mepparamtarget branch April 10, 2024 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Ready for Stage run-nala Run Nala Test Automation against PR trivial PR doesn't require E2E testing by a reviewer verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants