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

[MEP][AJO] POC to integrate AJO with MEP #3654

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

Conversation

markpadbe
Copy link
Contributor

@markpadbe markpadbe commented Feb 10, 2025

We previously did a POC to integrate AJO campaigns into MEP. But in the previous POC, manifests never had more than 1 column and there was no data that could have assisted us in choosing one column over another. This update enables AJO on the 404 page and prepares the code for later AJO adoption across pages.
NOTE: this update does not include integration with the enablePersV2.

Steps to test:

  1. navigate to the 404 page in an incognito browser
  2. edit the html of any anchor href(I used the adobe icon top left) to point to https://www.stage.adobe.com/products/photoshop/123.html and click on it
    the page should then reload
  3. Check the network tab for interact calls. If there are 2, it is likely working and you can continue to step 4 (you could also check the payload response for AJO content)
  4. overwrite files in the browser using the 3 files from this PR
  5. overwrite the launch file in marketingtech folder with contents of this file: https://assets.adobedtm.com/d4d114c60e50/a0e989131fd5/launch-2c94beadc94f-development.min.js
  6. Reload and see the updated content

NOTE: Before this code could work in production, the launch code would also need to be updated in production.

Resolves: MWPW-166638
Note: this was branch from MWPW-149470 and includes updates for that ticket as well.

Test URLs:

Copy link
Contributor

aem-code-sync bot commented Feb 10, 2025

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

@markpadbe markpadbe changed the title [MEP]{AJO] [MEP][AJO] POC to integrate AJO with MEP Feb 10, 2025
@markpadbe markpadbe closed this Feb 10, 2025
@markpadbe markpadbe reopened this Feb 10, 2025
Copy link
Contributor

aem-code-sync bot commented Feb 10, 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

@markpadbe markpadbe marked this pull request as draft February 10, 2025 04:03
@markpadbe markpadbe marked this pull request as ready for review February 10, 2025 18:37
Copy link
Contributor

@JasonHowellSlavin JasonHowellSlavin left a comment

Choose a reason for hiding this comment

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

Any tests that can be updated?

targetManifests.forEach((manifest) => {
manifest.source = ['target'];
targetAjoManifests.forEach((manifest) => {
manifest.source = [config.mep.ajoEnabled ? 'ajo' : 'target'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Some optional chaining could make this a bit more robust from errors, may not be necessary though:
config.mep?.ajoEnabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The config.mep is defined near the top of the init function, so this should be safe. If not, we would probably want to update all of the existing references to config.mep in the file.

@@ -1126,9 +1126,11 @@ async function checkForPageMods() {
const promo = getMepEnablement('manifestnames', PROMO_PARAM);
const target = martech === 'off' ? false : getMepEnablement('target');
const xlg = martech === 'off' ? false : getMepEnablement('xlg');
const isPOC = (document.title === '404' && window.location.pathname === '/products/photoshop/123.html');
Copy link
Contributor

@JasonHowellSlavin JasonHowellSlavin Feb 21, 2025

Choose a reason for hiding this comment

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

is there another way to do this? I am not particularly fond of having a hardcoded string for a path in the utils/utils.js file that is used by all consumers.

For instance, you could author custom page metadata within the metadatablock on a page by page basis to allow multiple POC pages

const isPOC = (document.title === '404' && getMetadata('ajoPoc'));

Copy link
Contributor Author

@markpadbe markpadbe Feb 21, 2025

Choose a reason for hiding this comment

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

hi @JasonHowellSlavin, as far as I know, the only way to impact the metadata for the 404 page is by updating the 404.html file via a code change. I tried authoring the metadata for the fragment that gets pulled in but that doesn't get applied to the page.

Copy link
Contributor

Choose a reason for hiding this comment

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

@markpadbe so the title check for 404 makes sense, but I don't think we want to lock it to the 1 URL. It should work on all 404 pages right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @vivgoodrich, the launch code that triggers the related propositionFetch call as of this week is targeting the /products/photoshop/123.html page. That could change, but it won't work on other pages unless that is also updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hear ya, but I think we want the POC to be able to work on more pages. Otherwise we'll have to do a PR later just to enable this later.

Copy link
Contributor Author

@markpadbe markpadbe Feb 22, 2025

Choose a reason for hiding this comment

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

Sure, but I've left open the option to test ajo on other pages by including including getMepEnablement('ajo') on line 30. It won't work on the 404 page because there isn't a way (that I'm aware of) to update the metadata on that page without a code update, but it would work on other pages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would a queryParam or hashParam work here and allow testing on all types of 404 pages? Or is there a reason that will not work as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasonHowellSlavin, I just got the ok to require a queryParam for the POC and I've updated the code. thanks.

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.

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.

3 participants