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-153580: Add Opt-In Feature for CaaS Badge Display #2625

Merged
merged 27 commits into from
Jul 29, 2024
Merged

Conversation

sanrai
Copy link
Contributor

@sanrai sanrai commented Jul 24, 2024

TLDR

We added a checkbox in CaaS tool configurator to enable a new badging feature.

Milo only contains the logic for toggling a checkbox true or false.

All the actual implementation is done on the CaaS side.

Note: This feature only affects an authoring tool. The LH scores tested by the GH actions are irrelevant in this context.

Description

This pull request introduces a new feature for Milo, allowing authors to opt-in to display badges on their CaaS cards. Previously, Milo was incorrectly sending the primaryTag information for each card. Specifically, for Milo (using the postXDM service), badge label information was being sent incorrectly as an ID (in the wrong structure), missing the tag's title and localized values.

To resolve this, we will use static tag data inside Chimera IO to look up the correct tag title based on the ID passed on by Milo. This approach is much easier than fixing the postXDM service and performing a database migration in OpenSearch.

By implementing this feature, Milo authors can now choose to display badges by enabling a checkbox.

Resolves: MWPW-153580

Additional Context

For cards from Dexter, badges should always display by default. For cards from Milo, authors may not expect badges unless they opt in. This prevents unintended badge displays for Milo authors who did not expect them (by accidentally authoring primaryTag fields without realizing they show badges)

Solution

Handling Different Content Origins:

We have consumers from Dexter and Milo.

  • For content from Dexter, badges should always show (default behavior).
  • For content from Milo, badges should only show if the author has opted in.

Code Changes:

  • Added a checkbox in the UI to allow Milo authors to opt-in for showing badges.

Updated Code

Backend Code:

const miloBadgeLabel = primaryTag.id ? localizeTag(primaryTag.id, language, country) : '';
...
{ label: { description: (primaryTag.title || miloBadgeLabel) } } : { label: { description: '' } }),

CaaS UI Code:

const showCardBadges = getConfig('collection', 'showCardBadges');
const fromDexter = origin === 'Dexter';
const showBadge = (isOneHalf || isThreeFourths || isFull) && (fromDexter || showCardBadges);

Milo Code:

showCardBadges: state.showCardBadges,
...
<${Input} label="Enable showing card badges (by default hidden)" prop="showCardBadges" type="checkbox" />

Testing

  • Verify that the opt-in checkbox is available for Milo authors.
  • Verify that badges are displayed correctly for Dexter content by default.
  • Verify that badges are displayed for Milo content only when opted in by the author.
  • Ensure that no badges are shown for Milo content if the author has not opted in.

Test URLs:

By merging this pull request, we will ensure that authors have control over badge visibility based on the content origin and their preferences.

@sanrai sanrai requested a review from chrischrischris as a code owner July 24, 2024 23:07

This comment was marked as duplicate.

@sanrai sanrai added the do not merge PR should not be merged yet label Jul 24, 2024

This comment was marked as outdated.

Copy link
Contributor

aem-code-sync bot commented Jul 25, 2024

Page Scores Audits Google
/tools/caas PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.70%. Comparing base (3afac8d) to head (547e097).
Report is 9 commits behind head on stage.

Additional details and impacted files
@@           Coverage Diff           @@
##            stage    #2625   +/-   ##
=======================================
  Coverage   95.69%   95.70%           
=======================================
  Files         169      172    +3     
  Lines       45295    45365   +70     
=======================================
+ Hits        43345    43416   +71     
+ Misses       1950     1949    -1     

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

@sanrai
Copy link
Contributor Author

sanrai commented Jul 25, 2024

Hi @overmyheadandbody,

I was in the process of raising the PR last night just before the end of the work day but didn’t have enough time to finish it up until today.

The PR is now complete with all the documentation written and tests updated. I have also addressed the failing checks and everything should be in order now :)

I see some failing checks that might deserve some attention. I also see this is marked as do not merge. Is it a draft? Will you be adding additional logic?

@sanrai sanrai added new-feature New block or other feature caas-configurator CaaS Configurator Ready for Stage and removed do not merge PR should not be merged yet labels Jul 25, 2024
@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Jul 26, 2024

Skipped merging 2625: MWPW-153580: Add Opt-In Feature for CaaS Badge Display due to failing checks

@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Jul 27, 2024

Skipped merging 2625: MWPW-153580: Add Opt-In Feature for CaaS Badge Display due to failing checks

@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Jul 28, 2024

Skipped merging 2625: MWPW-153580: Add Opt-In Feature for CaaS Badge Display due to failing checks

1 similar comment
@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Jul 29, 2024

Skipped merging 2625: MWPW-153580: Add Opt-In Feature for CaaS Badge Display due to failing checks

@mokimo mokimo merged commit 03980fd into stage Jul 29, 2024
24 of 25 checks passed
@mokimo mokimo deleted the MWPW-153580 branch July 29, 2024 09:29
@mokimo mokimo mentioned this pull request Jul 29, 2024
rohitsahu pushed a commit to rohitsahu/milo that referenced this pull request Jul 30, 2024
* stage:
  [MWPW-153611] [Gray Box] environment aware links (adobecom#2622)
  MWPW-153580: Add Opt-In Feature for CaaS Badge Display (adobecom#2625)
  [MWPW-154335] [callout] Spacing issue encountered when the call-out section is added (adobecom#2628)
  MWPW-150557 - Split Marquee CLS issues on consuming sites (adobecom#2636)
  Mwpw 147034: Custom border color + badge/border color decoupling [merch card] (adobecom#2613)
  [MWPW-151517] - Remove condition for promobar hidden on mobile from gnav (adobecom#2538)
  MWPW-154998 [MEP][MILO] Manifests do not execute in the right order when there is a disabled manifest (adobecom#2632)
  mwpw-154965: Fetch federal stage content from hlx.page instead of stage.adobe.com (adobecom#2618)
  Correct error messages for duplicate files on the stage to main workflow (adobecom#2621)
  MWPW-153245 [merch][analytics] dispatch wcomp events, and let default lh (adobecom#2610)
  Revert "MWPW-146528[MILO][MEP][ANALYTICS] Add attribute to content changed by Target for analytics and MWPW-152274" (adobecom#2627)
  MWPW-128600 Locale Tool: Langstore points to langstore/en (adobecom#2615)
  Fix for errors in dynamically loaded scripts in test cases (adobecom#2619)
  MWPW-146528[MILO][MEP][ANALYTICS] Add attribute to content changed by Target for analytics and MWPW-152274 (adobecom#2593)
  Bootstrapper script for milo feds blocks (adobecom#2560)
  Revert "[MWPW-152968] mWeb - Passing ECID to Branch.io banner - Implementation" (adobecom#2612)

# Conflicts:
#	libs/deps/merch-card.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caas-configurator CaaS Configurator new-feature New block or other feature Ready for Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants