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

CIF-1202 - Update the CIF Core Components to use configurations from /conf #184

Merged
merged 32 commits into from
Mar 12, 2020

Conversation

dplaton
Copy link
Contributor

@dplaton dplaton commented Jan 29, 2020

Description

The MagentoGraphqlClient is updated to read the configuration from the context, falling back to the old method (node properties) in case the configuration is missing.

The unit tests are updated as well and they are using the Sling Context-Aware Configuration Mock plugin which provides context-aware configuration capabilities to the Aem Context.

Related Issue

CIF-1202

Motivation and Context

Unify configuration of the CIF Connector and CIF Core Components

How Has This Been Tested?

Functional tests and 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.

Copy link
Contributor

@cjelger cjelger left a comment

Choose a reason for hiding this comment

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

Can you make sure that you don't automatically reformat the lines you don't edit? Otherwise your PR includes a lot of noise and it's hard to spot the changes related to the issue ... thx.

}
if (configBuilder != null) {
ValueMap properties = configBuilder.name(CONFIGURATION_NAME).asValueMap();
storeCode = properties.get(STORE_CODE_PROPERTY, readFallBackConfiguration(resource, STORE_CODE_PROPERTY));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as previous PR, only call the method if needed.

@@ -1,154 +1,160 @@
{
"pageA": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because your IDE reformatted the entire file, it's very hard to find out what you really modified ...

Configuration serviceConfiguration = configurationAdmin.getConfiguration(
"org.apache.sling.caconfig.resource.impl.def.DefaultContextPathStrategy");

Dictionary<String, Object> props = new Hashtable<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document what this does?... and isn't there anything better to setup this test from org.apache.sling.testing.caconfig-mock-plugin? I expected that the content you added to jcr-conf.json would be enough ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Sling CAConfig API uses different property names (sling:configs instead of settings,sling:configRef instead of cq:conf etc) so I configured the mock components as they are in AEM. Of course, I could've just used those Sling properties in the test JSON but I wanted the test content to be as close as possible to our AEM use case.

Copy link
Contributor

@cjelger cjelger Jan 29, 2020

Choose a reason for hiding this comment

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

Thanks for the info ... just out of curiosity, how is this configured then in AEM? I tried to find a config for caconfig and that kind of things but couldn't find anything that would for example define what you do here "by hand".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(one eternity later...)

There is a bunch of Apache Sling Context-Aware Configuration ... components at http://localhost:4502/system/console/configMgr

- optimize the "fallback solution" so that the fallback configuration is only read when the context configuration is missing
@codecov
Copy link

codecov bot commented Jan 29, 2020

Codecov Report

Merging #184 into master will increase coverage by 0.13%.
The diff coverage is 86.48%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #184      +/-   ##
============================================
+ Coverage     59.92%   60.05%   +0.13%     
- Complexity      532      537       +5     
============================================
  Files           147      147              
  Lines          4344     4366      +22     
  Branches        738      744       +6     
============================================
+ Hits           2603     2622      +19     
  Misses         1652     1652              
- Partials         89       92       +3
Flag Coverage Δ Complexity Δ
#jest 38.21% <ø> (ø) 0 <ø> (ø) ⬇️
#karma 94.97% <ø> (ø) 0 <ø> (ø) ⬇️
#unittests 84.79% <86.48%> (+0.02%) 537 <5> (+5) ⬆️
Impacted Files Coverage Δ Complexity Δ
...ernal/servlets/GraphqlClientDataSourceServlet.java 89.74% <85.71%> (-1.17%) 7 <0> (+1)
...e/core/components/client/MagentoGraphqlClient.java 92.3% <86.36%> (-0.8%) 11 <4> (+2)
.../internal/models/v1/navigation/NavigationImpl.java 86.06% <87.5%> (-0.15%) 25 <1> (+2)

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 1b9f763...5860454. Read the comment docs.

@herzog31 herzog31 added the feature New feature or request label Jan 30, 2020
}
;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this line

@LSantha LSantha merged commit 55b044a into master Mar 12, 2020
@LSantha LSantha deleted the issue/CIF-1134 branch March 12, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request To Verify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants