Skip to content

Conversation

@michielspiritus
Copy link

Q                       A
Fixed Issues? Fixes #2625
Minor: New Feature
Documentation Provided Yes (code comments and or markdown)

@codecov
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 87.32%. Comparing base (e333b37) to head (7edd3ce).

❗ Current head 7edd3ce differs from pull request most recent head 2efb0f5. Consider uploading reports for the commit 2efb0f5 to get more accurate results

Files Patch % Lines
...m/adobe/cq/wcm/core/components/internal/Utils.java 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2626      +/-   ##
============================================
+ Coverage     87.21%   87.32%   +0.10%     
+ Complexity     2665     2653      -12     
============================================
  Files           233      232       -1     
  Lines          7123     7098      -25     
  Branches       1085     1080       -5     
============================================
- Hits           6212     6198      -14     
+ Misses          362      357       -5     
+ Partials        549      543       -6     

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

@vladbailescu
Copy link
Member

@michielspiritus , thank you for the contribution!

I understand this is an important improvement for one of your customers. To better integrate it with the existing functionality, I believe the option of "Inherit featured image from page" should be removed (or at least disabled) when the inheritance is disabled in the policy.

Also, to speed review and merge of this PR, may I kindly ask you to also provide an integration test? We already have tests covering the existing functionality at https://github.com/adobe/aem-core-wcm-components/blob/main/testing/it/e2e-selenium/src/test/java/com/adobe/cq/wcm/core/components/it/seljup/tests/image/v3/ImageIT.java

String fileReference = properties.get(DownloadResource.PN_REFERENCE, String.class);
Resource fileResource = resource.getChild(DownloadResource.NN_FILE);
boolean imageFromPageImage = properties.get(PN_IMAGE_FROM_PAGE_IMAGE, StringUtils.isEmpty(fileReference) && fileResource == null);
boolean disblePageImageInheritance = currentStyle != null ? currentStyle.get(Image.PN_PAGE_IMAGE_INHERITANCE_DISABLED, false) : false;
Copy link
Member

Choose a reason for hiding this comment

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

Small typo in variable name, I believe it should be disablePageImageInheritance

@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@michielspiritus
Copy link
Author

I've wrote some IT tests but it's hard to test, always need to downgrade chrome. Can you review my latest changes?

@vladbailescu vladbailescu requested a review from LSantha February 29, 2024 10:16
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@koenkicken
Copy link

@vladbailescu any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow disabling the inheritance of the featured image in the image component

5 participants