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-163379: Fix edit url from Helix admin API for DA #3314

Merged
merged 6 commits into from
Jan 2, 2025

Conversation

sharmeeatadobe
Copy link
Contributor

@sharmeeatadobe sharmeeatadobe commented Dec 5, 2024

  • Admin API for helix throws 404 for edit block for DA authored page. But it provides the DA content link in the preview.sourceLocation key
  • Since Helix does not support this for DA yet and does not have an ETA, as per the DA product team's recommendation, reading preview.sourceLocation and replacing with the edit url for DA.
  • Added an icon for Adobe DA experience cloud to show on the preflight modal for DA author page

Resolves: MWPW-163379

Test URLs:

image
  • After (With DA changes)
image

After

Copy link
Contributor

aem-code-sync bot commented Dec 5, 2024

Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

I'm confused. There's a da-bacom link before and milo link after ... those are two different projects with different content. Using the milolibs param, you should be able to consume your branch's sidekick on a consumer

libs/blocks/preflight/preflight.css Outdated Show resolved Hide resolved
libs/blocks/preflight/preflight.css Show resolved Hide resolved
libs/blocks/preflight/preflight.css Show resolved Hide resolved
libs/blocks/preflight/panels/general.js Outdated Show resolved Hide resolved
@sharmeeatadobe
Copy link
Contributor Author

I'm confused. There's a da-bacom link before and milo link after ... those are two different projects with different content. Using the milolibs param, you should be able to consume your branch's sidekick on a consumer

The issue is reported on the 'da-bacom' which uses content from DA. The after link for Milo is specifically for checking regression since the code change will be affecting both milo and da-bacom sites. (Refer the heading "Regression" above the after URL).

@sharmeeatadobe sharmeeatadobe requested a review from mokimo December 5, 2024 13:40
@mokimo
Copy link
Contributor

mokimo commented Dec 5, 2024

This would be your after link https://main--da-bacom--adobecom.aem.page/drafts/ancy/content-analytics?milolibs=mwpw-163379--milo--sharmeeatadobe

you can have multiple before/after links for each project/consumer.

@sharmeeatadobe
Copy link
Contributor Author

This would be your after link https://main--da-bacom--adobecom.aem.page/drafts/ancy/content-analytics?milolibs=mwpw-163379--milo--sharmeeatadobe

you can have multiple before/after links for each project/consumer.

Great! Thankful for this @mokimo , let me update

Comment on lines 21 to 27
function getDAEditUrl(sourceUrl) {
if (!sourceUrl) {
return '';
}
const daEditUrl = sourceUrl.replace('markup:https://content.da.live', 'https://da.live/edit#');
return daEditUrl;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be simplified?

function getDAEditUrl(sourceUrl) {
  return sourceUrl?.replace('markup:https://content.da.live', 'https://da.live/edit#') || '';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sharg1 made the suggested change

Copy link
Contributor

github-actions bot commented Dec 6, 2024

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.

@sharmeeatadobe sharmeeatadobe requested a review from sharg1 December 6, 2024 05:21
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.48%. Comparing base (d543279) to head (4e5f78a).
Report is 7 commits behind head on stage.

Additional details and impacted files
@@           Coverage Diff           @@
##            stage    #3314   +/-   ##
=======================================
  Coverage   96.48%   96.48%           
=======================================
  Files         254      254           
  Lines       59031    59031           
=======================================
+ Hits        56954    56955    +1     
+ Misses       2077     2076    -1     

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

@narcis-radu narcis-radu requested a review from a team December 12, 2024 09:01
@sharmeeatadobe sharmeeatadobe requested review from robert-bogos and a team December 13, 2024 10:07
@sharg1
Copy link
Contributor

sharg1 commented Dec 16, 2024

hello @afmicka @skumar09 - i'm seeing few nala checks failing wrt to MAS tests.. Doesn't seem to be related to this PR. Could you please check? cc @mokimo

@mokimo
Copy link
Contributor

mokimo commented Dec 16, 2024

What I usually do to debug failing nala tests:

  1. Purge the branch CURL -si -X POST https://admin.hlx.page/code/sharmeeatadobe/milo/mwpw-163379/*
  2. Restart the tests after a ~30s (purging is happening asynchronous)
  3. Try to rebase to stage if the branch seems outdated
  4. Check the tests, see which ones are failing, locate the page and try to find out why its not worrking

@sharmeeatadobe
Copy link
Contributor Author

What I usually do to debug failing nala tests:

  1. Purge the branch CURL -si -X POST https://admin.hlx.page/code/sharmeeatadobe/milo/mwpw-163379/*
  2. Restart the tests after a ~30s (purging is happening asynchronous)
  3. Try to rebase to stage if the branch seems outdated
  4. Check the tests, see which ones are failing, locate the page and try to find out why its not worrking

Although the Nala tests passed after the latest pull from stage, there are unrelated failures in UTs for HelixReview.test.js
Seems like a timeout error which needs a re-run. @mokimo please help with re-running the job
Ran it in local and it passes
image

Copy link
Contributor

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.

@@ -165,13 +169,14 @@ function Item({ name, item, idx }) {
const { publishText, disablePublish } = usePublishProps(item);
const isChecked = item.checked ? ' is-checked' : '';
const isFetching = item.edit ? '' : ' is-fetching';
const editIcon = item.edit && item.edit.includes(DA_DOMAIN) ? 'da-icon' : 'sharepoint-icon';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
const editIcon = item.edit && item.edit.includes(DA_DOMAIN) ? 'da-icon' : 'sharepoint-icon';
const editIcon = item.edit?.includes(DA_DOMAIN) ? 'da-icon' : 'sharepoint-icon';

@milo-pr-merge milo-pr-merge bot merged commit 01ffecc into adobecom:stage Jan 2, 2025
19 checks passed
@milo-pr-merge milo-pr-merge bot mentioned this pull request Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants