acs-aem-commons icon indicating copy to clipboard operation
acs-aem-commons copied to clipboard

CCVAR: Fixed Same Attribute not updating correctly.

Open vabs95 opened this issue 11 months ago • 6 comments

Defect: CCVAR Same Attribute doesn't update all placeholders in the value.

Uses the original attribute value and only updates the last placeholder value leaving the other placeholders as it is.

Example:

Attr "title=((custom_prefix)) ((custom_title))"

Expected: "title=Prefix Title"

Actual: "title=((custom_prefix)) Title"

Solution: Update the currentAttr Value with new attrValue when looping placeholders.

vabs95 avatar Feb 26 '24 12:02 vabs95

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 55.45%. Comparing base (7676ed5) to head (9d78151). Report is 19 commits behind head on master.

:exclamation: Current head 9d78151 differs from pull request most recent head 2000b24

Please upload reports for the commit 2000b24 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3275      +/-   ##
============================================
+ Coverage     55.34%   55.45%   +0.11%     
+ Complexity     5543     5506      -37     
============================================
  Files           728      721       -7     
  Lines         29705    29535     -170     
  Branches       3875     3840      -35     
============================================
- Hits          16440    16379      -61     
+ Misses        11722    11630      -92     
+ Partials       1543     1526      -17     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 26 '24 12:02 codecov[bot]

@kwin @csaboka @davidjgonzalez I added this fix, can you please review.

Also, I added test case but it still failing. Can you please tell me what am I missing here?

vabs95 avatar Feb 26 '24 13:02 vabs95

@vabs95 : JUnit 4 tests are not running at the moment, I'm trying to fix that in #3273 . Since the test you are trying to extend uses JUnit 4 tools, it doesn't run at all and won't contribute to coverage no matter what you change.

You can either wait until #3273 is merged, or try rewriting the test class to JUnit 5.

csaboka avatar Feb 26 '24 17:02 csaboka

@csaboka Can you please review it now?

vabs95 avatar Mar 06 '24 16:03 vabs95

@vabs95 : I'm not a project owner, just a minor contributor to the project. I'm not familiar with the code you touched, but even if I were, my approval wouldn't be enough to allow merging this.

csaboka avatar Mar 06 '24 16:03 csaboka

@kwin can you please review it ?

vabs95 avatar Mar 07 '24 10:03 vabs95

@davidjgonzalez can you please re-run the checks?

vabs95 avatar Jul 08 '24 10:07 vabs95

hi @davidjgonzalez can you please merge this now? It has been pending for a long time.

vabs95 avatar Aug 05 '24 13:08 vabs95