-
Notifications
You must be signed in to change notification settings - Fork 169
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-134799: MEP On/Off #1580
MWPW-134799: MEP On/Off #1580
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
|
|
let event = null; | ||
const scheduled = manifestPath.match(REGEX_PROMO); | ||
if (scheduled) { | ||
event = events.find((e) => e.name === scheduled[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is 1? could we use a talkative constant (PROMO_NAME_GRP?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constant is now gone completely because of a changed approach
for (const manifest of cleanedManifests) { | ||
results.push(await runPersonalization(manifest, config)); | ||
const noOverride = !config.mep?.override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be clearer to have an override flag, and then !override test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
const REGEX_PROMO = /\/promos\/(\w+)\//; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is \w+ enough for first level folder? characters like '-' or '_' are allowed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good point, this regex certainly needs more thinking.
let me research a bit more and fix it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed regex at all
disabled = true; | ||
} else { | ||
const currentDate = new Date(); | ||
if (event.start && event.end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we want to manage "evergreen" promotion that are endless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now, i don't recall such use-case.
can you give more details?
how will it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind, just checked, and those use cases are not meant to leverage mep anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we want to manage "evergreen" promotion that are endless?
Evergreen sounds like regular content then, no? Should not be tied to promotion anymore in that case.
|
|
|
|
|
|
|
for now rebased on |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1580 +/- ##
==========================================
- Coverage 95.65% 95.05% -0.60%
==========================================
Files 150 153 +3
Lines 38783 39170 +387
==========================================
+ Hits 37097 37234 +137
- Misses 1686 1936 +250 ☔ View full report in Codecov by Sentry. |
@3ch023 et al... as of today i finally have all my "tests" up and running in the new system, and so far it is a huge step forward! I have various promos (marquee, segment, medium promo, sticky, etc) for various campaigns (bf, bts, ste). I've updated the master "promotions" spreadsheet manifest with a matrix we can use for our yearly cycle... so yes... it all feels right, so far... |
sure, moved it in both CC and Milo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
As discussed offline, i would have preferred i think to leave promotion semantic out of mep completely. metadata name manifestpaths
goes in that route. Ideally we could have mep having the feature to override bits of the site at schedule moments, and commerce users using that feature for promotion purpose.
I keep in mind though promotion is our main driver, and making things generic is not always the best route at the beginning. Let's see what usage gives us
* MWPW-134799: add on/off to MEP
* MWPW-134799: add on/off to MEP
* MWPW-134799: add on/off to MEP
MEP On/Off
This PR introduces a client-side on/off feature for MEP manifests.
Related Discussion
More details below.
Resolves:
Test URLs:
Details
Promotions.xlsx
Promotions.xlsx - a separate global metadata file, configured in
.helix/config.xlsx
folder/page
-manifest
and on/off date times.'scheduled'
. It is auto-generated from the 'schedule' tab and only visible when you preview promotions.xlsx.Why a separate metadata file?
Schedule Tab
Priority
Promotions.xlsx is higher priority than metdata.xlsx
Promotions.xlsx only allows 2 fields to be configured, 'personalization' and 'events'. Its protected, so no other column from global metadata can be overwritten. On the contrary, global metadata should not be able to overwrite the 'personalization' and 'events' data in promotions.xlsx.
Priority is achieved due to order in .helix/config.xlsx
'manifestnames' and 'schedule' meta tags
manifest name
|start datetime (GMT)
|end datetime (GMT)
|manifest path
Sample page
MEP behavior
Preview
'Scheduled - inactive/active' shows if the current browser time is within the manifest's on/off period.
On and Off date time is shown in the local timezone, NOT in GMT.
When there is a manifest overwrite in the URL, MEP will disregard the schedule. This is needed for QA-ing inactive changes.