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

solves #2625: Add feature to disable the featured page image inheritance #2626

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

michielspiritus
Copy link

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

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

@@ -344,9 +345,10 @@ public static Resource getWrappedImageResourceWithInheritance(Resource resource,
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

Copy link

sonarcloud bot commented Dec 20, 2023

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?

Copy link

sonarcloud bot commented Feb 29, 2024

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