Skip to content

Conversation

@laurentiumagureanu
Copy link
Contributor

Description

XFs are to be tagged with the category identifier (ID or UID) as per CA config.
If the URLProvider is configured to use another category field, then a graphQL query is executed to fetch the right identifier.
Same applies for products

Related Issue

CIF-2034

Motivation and Context

The current XF placeholder component is limited in finding related XF for product or category pages based on the URL selector only. This requires the URL selector used to be the same the XFs are tagged with.

How Has This Been Tested?

Unit tests

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #565 (484e098) into master (41637fa) will decrease coverage by 0.06%.
The diff coverage is 73.91%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #565      +/-   ##
============================================
- Coverage     87.45%   87.38%   -0.07%     
- Complexity     1294     1301       +7     
============================================
  Files           235      237       +2     
  Lines          6059     6089      +30     
  Branches        888      900      +12     
============================================
+ Hits           5299     5321      +22     
- Misses          587      588       +1     
- Partials        173      180       +7     
Flag Coverage Δ
integration 65.13% <39.13%> (-0.17%) ⬇️
jest 83.49% <ø> (ø)
karma 94.82% <ø> (ø)
unittests 87.20% <73.91%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/internal/models/v1/product/ProductRetriever.java 100.00% <ø> (ø)
...models/v1/experiencefragment/ProductRetriever.java 71.42% <71.42%> (ø)
...riencefragment/CommerceExperienceFragmentImpl.java 75.18% <72.41%> (-0.71%) ⬇️
...odels/v1/experiencefragment/CategoryRetriever.java 80.00% <80.00%> (ø)
...nts/models/retriever/AbstractProductRetriever.java 89.13% <0.00%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41637fa...484e098. Read the comment docs.


private Resource xfResource;
private String name;
private AbstractCategoryRetriever categoryRetriever;
Copy link
Member

Choose a reason for hiding this comment

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

Normally the retrievers should be exposed via getters, so they can be accessed when applying Sling model delegation. But I don't think it's needed here, since the retrievers are only required for identifier lookup and I don't see a use case why the query would be extended.

@herzog31 herzog31 requested a review from buuhuu May 12, 2021 07:47

if (VALID_CATEGORY_IDENTIFIERS.contains(identifier.getLeft())) {
categoriesIdentifier = identifier.getRight();
if (identifier.getRight() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

VALID_CATEGORY_IDENTIFIERS is now unused and may be removed.

@laurentiumagureanu laurentiumagureanu requested a review from buuhuu May 19, 2021 14:41
Base automatically changed from issues/CIF-2032 to master May 28, 2021 15:38
@LSantha LSantha merged commit fcc1917 into master May 28, 2021
@LSantha LSantha deleted the issues/CIF-2034 branch May 28, 2021 18:16
@LSantha LSantha added the enhancement New feature or request label May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants