[MAGNOLIA-6606] Variants Not Marked as Modified when Audience Selected Created: 03/Feb/16  Updated: 23/May/16  Resolved: 28/Mar/16

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: 5.4.3
Fix Version/s: 5.4.6

Type: Bug Priority: Neutral
Reporter: Richard Gange Assignee: Oanh Thai Hoang
Resolution: Fixed Votes: 0
Labels: support
Remaining Estimate: 0d
Time Spent: 4d 7h
Original Estimate: 5d

Attachments: PNG File screen-shot.png    
Issue Links:
Cloners
is cloned by MAGNOLIA-6642 CLONE - Variants Not Marked as Modifi... Closed
Relates
dependency
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Saigon 37
Story Points: 8

 Description   

Steps to reproduce

  1. Create a variant
  2. Update the name field or add an additional segment in the Audience field
  3. Click PICK
  4. Observe activation status of the node
    => Expected: It is marked modified and changed to yellow.
    => Actual: It is still green.

Solution

Modify the function #shouldIgnoreUpdate() in main to support ignore mgn:variantTitle, variationOf and mgnl:assignedSegments...



 Comments   
Comment by Oanh Thai Hoang [ 25/Feb/16 ]

Root cause:
info.magnolia.personalization.variant.VariantManager in Personalization module defined 3 properties which names start by mgnl and
info.magnolia.jcr.wrapper..MgnlPropertySettingContentDecorator in Core module provides ability to ignore update mgnl:lastModified if those properties have name start with "mgnl" or "jcr"

Solution: There are 3 ways to fix it: modify function shouldIgnoreUpdate in Core module or fix wrong property name in Personalization module

1/ Modify the function shouldIgnoreUpdate to support ignore mgn:variantTitle and mgnl:assignedSegments... It's a easy and simple solution but have one weakness: if user change the name of property, it couldn't work as expectation

  • File change: info.magnolia.jcr.wrapper.MgnlPropertySettingContentDecorator

+ Define the constants

    /**
     * List contains special property Names that we should force to update last update time whenever they change.
     */
    private final static List<String> specialPropertyNames = Arrays.asList(
            NodeTypes.MGNL_PREFIX + "variantTitle",
            NodeTypes.MGNL_PREFIX + "variationOf",
            NodeTypes.MGNL_PREFIX + "assignedSegments",
            NodeTypes.Renderable.TEMPLATE
    );
    /**

+ And change the function to unignore list of special property names

     protected boolean shouldIgnoreUpdate(final String propertyName) {
         // our update doesn't count as update (activation, versioning, last update, etc) - except for changing the template
        return propertyName.startsWith(NodeTypes.JCR_PREFIX) || (propertyName.startsWith(NodeTypes.MGNL_PREFIX) && !specialPropertyNames.contains(propertyName));
     }

2/ Create SaveChooseAudienceDialogAction to update mgnl:lastModified property

  • Files change:

+ Create SaveChooseAudienceDialogAction extends from SaveDialogAction. This class is used to update the lastModified property:

NodeTypes.LastModified.update(node);

+ Change bootstrap file: magnolia-personalization-integration/src/main/resources/mgnl-bootstrap/personalization-integration/config.modules.personalization-integration.dialogs.chooseAudience.xml to update new save action class
+ Add task to update node commit also

3/ Fix wrong property name in Personalization module. It is a complex solution

  • Files change:
    + info.magnolia.personalization.variant.VariantManager to change the property names
    + Resource file: personalization/magnolia-personalization-integration/src/main/resources/mgnl-nodetypes/magnolia-personalization-nodetypes.xml. Change node type with new property names
    + Change some bootstrap files in personalization-segmentation-app and personalization-integration
    + Add task to rename name of those properties when update to version 1.2.4 in personalization-segmentation-app and personalization-integration
    + Need to write a task to search and update old value of variants in website workspace due to personalization module integrated with Pages app.
Comment by Oanh Thai Hoang [ 25/Feb/16 ]

Hi, mgeljic]. Could you please have a look and approve for one of my solution above. Thanks in advance.

Comment by Mikaël Geljić [ 29/Feb/16 ]

Right, this is an interesting and tricky one

I just sat with pmundt to consider those options, here's what we can say for now:

  • Option 1: albeit the easiest one, we're not too keen on having personalization peculiarities "leak" into core
  • Option 3 is a no-go, for example `mgnl:variantTitle` is a totally legit property name, + we'd rather avoid the impact & content migration of such rename.
  • We could live with option 2
  • Option 4: We're also considering opening up configurability of those "non-excluded" properties/nodes, like `mgnl:template` currently is in the `ContentDecorator`

I try to clarify feasibility of such option 4. If it's a dead-end, we may go for option 2.

Comment by Mikaël Geljić [ 17/Mar/16 ]

Well, ok, option 4 is not realistic in a timely manner (we've got better fish to fry atm), but I started to think I could live with option 1 as well:

  • It would have the most logical transition, if we eventually tackle option 4 later
  • one could argue that even mgnl:template shouldn't be hard-coded in there either, so as long as we keep same things grouped together I'm fine
  • reducing workaround/debt code to a minimum, compared to option 2

pmundt any opinion? Basically you can make the final call now

Comment by Philip Mundt [ 18/Mar/16 ]

mgeljic I agree with you. I could also live with option 1.

Comment by Mikaël Geljić [ 18/Mar/16 ]

And got the OK here, provided we don't make public constants out of it (specialPropertyNames is private so we're good on that end).

Finally, just for the mention: option 5 Making this list of specialPropertyNames the opposite one, i.e. properties for which we *do* want to ignore the update. As mentioned in inline comment:

// our update doesn't count as update (activation, versioning, last update, etc)

I doubt that set is gonna be any bigger.

Comment by Oanh Thai Hoang [ 21/Mar/16 ]

Reopen due to changing approach. We should pick up option change in main only

Generated at Mon Feb 12 04:16:06 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.