Skip to content

Conversation

@laurentiumagureanu
Copy link
Contributor

Description

Adds support for generating category URLs when UID support is enabled or UID is used as selection for custom category pages

Related Issue

CIF-2022

Motivation and Context

Adopt Magento GraphQL schema changes - Category UID

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.

@laurentiumagureanu laurentiumagureanu changed the base branch from master to bugs/CIF-2030 April 7, 2021 15:05
Copy link
Member

@herzog31 herzog31 left a comment

Choose a reason for hiding this comment

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

Do we have some kind of documentation of migration guide, when a customer should start using uidAndUrlPath?

Base automatically changed from bugs/CIF-2030 to master April 8, 2021 09:49
@laurentiumagureanu
Copy link
Contributor Author

@herzog31 CIF-2021 should cover the docs

.position()
.image();

if (enableUIDSupport || identifierType == UrlProvider.CategoryIdentifierType.UID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also only query id if we are using it. See line 36.

Copy link
Member

Choose a reason for hiding this comment

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

The condition here is for if it's supported to query a category by UID. So you can query for the UID anytime, no condition needed.

.map();
.urlPath(category.getUrlPath());

if (enableUIDSupport || categoryIdentifierType == CategoryIdentifierType.UID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, if uid is used we should not add id to the params.

Copy link
Member

Choose a reason for hiding this comment

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

At this point we don't know. It depends on the configuration of the URLProvider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Arg, yes you are right we must query all options used by the URLProvider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the URLProvider should provide a method to return how the configuration is and what attribute is needed, this can then be used by the query.

@mhaack mhaack added the bug Something isn't working label Apr 12, 2021
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #538 (617057c) into master (47342bf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #538   +/-   ##
=========================================
  Coverage     87.41%   87.42%           
  Complexity     1268     1268           
=========================================
  Files           232      232           
  Lines          5968     5971    +3     
  Branches        878      879    +1     
=========================================
+ Hits           5217     5220    +3     
  Misses          583      583           
  Partials        168      168           
Flag Coverage Δ Complexity Δ
integration 64.08% <40.00%> (+<0.01%) 0.00 <4.00> (ø)
jest 83.33% <ø> (ø) 0.00 <ø> (ø)
karma 94.54% <ø> (ø) 0.00 <ø> (ø)
unittests 87.52% <100.00%> (+0.01%) 0.00 <4.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
.../components/internal/services/UrlProviderImpl.java 94.17% <100.00%> (+0.17%) 44.00 <4.00> (ø)
...ponents/internal/servlets/SpecificPageServlet.java 100.00% <100.00%> (ø) 5.00 <0.00> (ø)
...commerce/core/components/utils/SiteNavigation.java 84.50% <100.00%> (ø) 33.00 <0.00> (ø)

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 47342bf...617057c. Read the comment docs.

@mhaack mhaack merged commit 73fc10b into master Apr 14, 2021
@mhaack mhaack deleted the issues/CIF-2022 branch April 14, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants