CIF-1447 - The Core CIF Components must use a service user to access configurations #300
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…configurations Add the service users mapping to the content package. Implement a `ComponentsConfigurationProvider` which reads the context configuration data using the service users. Update `MagentoGraphqlClient` to use the new service when reading configuration data. Update some models to use the new service to read configuration data.
mhaack
reviewed
Jun 12, 2020
...java/com/adobe/cq/commerce/core/components/internal/models/v1/navigation/NavigationImpl.java
Outdated
Show resolved
Hide resolved
added 7 commits
June 15, 2020 14:24
…configurations Remove the `ComponentsConfigurationProvider` service and implement a `ComponentsConfigurationAdapterFactory` which adapts from a `Resource` to a `ComponentsConfiguration` object. The `ComponentsConfiguration` is just a wrapper around a `ValueMap` and it's public so it can also be used in an HTL script it future use-cases require.
…configurations Update `MagentoGraphqlClient` to use the new `ComponentsConfigurationAdapter`
…configurations Update models and unit tests to use the new method of reading the configurations.
…configurations Add unit tests for `ComponentsConfigurationAdapterFactory`.
…configurations Fix unit test after merging with the `master` branch
dplaton
requested review from
cjelger,
LSantha,
herzog31,
hbacila and
laurentiumagureanu
Codecov Report
@@ Coverage Diff @@
## master #300 +/- ##
============================================
- Coverage 64.77% 64.62% -0.15%
- Complexity 802 809 +7
============================================
Files 179 181 +2
Lines 5601 5631 +30
Branches 875 875
============================================
+ Hits 3628 3639 +11
- Misses 1848 1864 +16
- Partials 125 128 +3
Continue to review full report at Codecov.
|
cjelger
reviewed
Jun 16, 2020
| /* | ||
| * Copyright 2020 Adobe. All rights reserved. | ||
| * | ||
| * This file is licensed to you under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
Sorry to be picky, this header is not formatted like all the other headers we use.
...obe/cq/commerce/core/components/internal/services/ComponentsConfigurationAdapterFactory.java
Show resolved
Hide resolved
...re/src/main/java/com/adobe/cq/commerce/core/components/services/ComponentsConfiguration.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/adobe/cq/commerce/core/components/services/ComponentsConfiguration.java
Outdated
Show resolved
Hide resolved
...q/commerce/core/components/internal/models/v1/categorylist/FeaturedCategoryListImplTest.java
Outdated
Show resolved
Hide resolved
...t/java/com/adobe/cq/commerce/core/components/internal/models/v1/product/ProductImplTest.java
Outdated
Show resolved
Hide resolved
...om/adobe/cq/commerce/core/components/internal/models/v1/productlist/ProductListImplTest.java
Outdated
Show resolved
Hide resolved
...cq/commerce/core/components/internal/services/ComponentsConfigurationAdapterFactoryTest.java
Outdated
Show resolved
Hide resolved
cjelger
reviewed
Jun 17, 2020
…configurations Update unit test
herzog31
approved these changes
Jun 17, 2020
…configurations Follow-up on the PR review: - fix copyright headers - code cleanup - fix the graphql-requests.log file - clean up the ProductImplTest which was passing, but for the wrong reasons.
cjelger
reviewed
Jun 17, 2020
| * @return a {@link ValueMap} object. | ||
| */ | ||
| public ValueMap getValueMap() { | ||
| Map<String, Object> temp = new HashMap<>(); |
There was a problem hiding this comment.
I think you can just write new HashMap<>(internalProperties) because ValueMap implements Map.
cjelger
approved these changes
Jun 17, 2020
…configurations Optimize the `getValueMap` method of `ComponentsConfiguration`
…nents into issues/CIF-1447
LSantha
reviewed
Jun 18, 2020
| ValueMap properties = configBuilder.name("cloudconfigs/commerce").asValueMap(); | ||
| rootCategoryId = properties.get(PN_MAGENTO_ROOT_CATEGORY_ID, Integer.class); | ||
| } | ||
| ComponentsConfiguration properties = catalogPage.adaptTo(ComponentsConfiguration.class); |
There was a problem hiding this comment.
Will that work? (catalogPage is not a Resource)
There was a problem hiding this comment.
It probably won't. I didn't notice it in the tests because it always falls back to the context configuration.
There was a problem hiding this comment.
Actually that component reads the configuration in the "opposite" order of what we want to do: it should first get the config from that new ComponentsConfiguration and then fallback to the old way ... that's probably why the test didn't fail.
There was a problem hiding this comment.
Yes, this is the way it's intended for this component - it first reads the category from the page properties and then it falls back to the configuration.
|
Following the last comment from @LSantha , I realised you probably also need to update |
…configurations Update the `StoreConfigurationExporter` to also use `ComponentsConfiguration`
* The `try-with-resource` pattern that I tried to use in the `ComponentsConfigurationAdapterFactory` is cool, but it fails in this case because the `ValueMap` used to build the `ComponentsConfiguration` object is unusable if the resolver is closed. I switched to using a service resolver created in the `activate()` method. * Update the `StoreConfigExporter` to use the `ComponentsConfiguration` object * Fix a bug in the `NavigationImpl` model which was causing the model not to fallback to the page hierarchy in case of a missing `magentoRootCategoryId` property in the context configuration.
LSantha
approved these changes
Jun 18, 2020
added 3 commits
June 18, 2020 16:29
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add the service user mapping to the content package.
Implement an adapter factory that adapts a resource to a specific configuration object using a service resolver. I chose this approach because we cannot adapt directly to a
ValueMap.The configuration object is a wrapper around the
ValueMapand it's in the public space, so HTL scripts can use them if needed.The
MagentoGraphqlClienthas been updated to use this new method.Related Issue
CIF-1447
Motivation and Context
Align our implementation with the security requirements that state that all access to
/confshould be done via service users, not directly manipulating the ACLsHow Has This Been Tested?
Functional testing, unit tests
Screenshots (if appropriate):
Types of changes
Checklist: