[MGNLPN-402] Trait field: voters adapters are not properly linked to their parent Created: 30/Sep/15  Updated: 24/Jan/18  Resolved: 01/Nov/17

Status: Closed
Project: Magnolia Personalization
Component/s: None
Affects Version/s: 1.2.10
Fix Version/s: 1.2.12, 1.3.6, 1.4.7

Type: Bug Priority: Major
Reporter: Sander K Assignee: Antonín Juran
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: PNG File Screen Shot 2017-10-13 at 14.31.45.png     PNG File Screen Shot 2017-10-13 at 14.31.57.png     XML File config.modules.sonera-fi-personalization.traits.xml     PNG File sampleTrait.png     Zip Archive sonera-fi-personalization.zip    
Issue Links:
Cloners
is cloned by MGNLPN-405 Trait field: voters adapters are not ... Closed
causality
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: Kromeriz 112, Kromeriz 113, Kromeriz 115, Kromeriz 116, Kromeriz 117, Kromeriz 118, Kromeriz 119, Kromeriz 120, Kromeriz 121
Story Points: 21

 Description   

I created new trait with definition like in attached picture. When I am selecting audiance for page variation and this is only trait I select and I leave checkbox un-checked then audiance info is not saved in Magnolia repository.
If I check the box and save it and then remove check by editing then audiance info is in repository also if I combine it with other traits I get expected results.

Also attached configuration xml and module code. Module code needs some tweaking to run in your environment, but I hope this might help with recreating the issue.



 Comments   
Comment by Antti Hietala [ 24/Aug/17 ]

This looks like a bug. An unselected checkbox should be stored as value false. Even if you set the defaultValue property on the field it is not stored as false. We propose to change the behavior of binary fields like checkbox so that an unselected choice is stored as false.

This issue is not specific to personalization although the ticket was originally entered as such. The issue should be fixed in UI.

Comment by Milan Divilek [ 08/Sep/17 ]

Reopen: Unfortunately fix doesn't work...

Reproduce:
1. Go to pages app and open /travel/stories page
2. Edit page property
3. Save dialog without manipulating with "Navigation" checkbox
4. Got to JCR browser app
5. "/travel/stories@hideInNav" property doesn't exist

Comment by Mikaël Geljić [ 20/Sep/17 ]

Hello there, I think this issue might have been misunderstood, can you enlighten me?

In one of the last comments from Thomas on the support ticket, he gives reproduction steps on the demo... with the select field. Not checkbox. Judging from the commits, I doubt this one is gone.

As such, it was *not* a problem that checkboxes don't store any property when they have no defaultValue (one can still configure defaultValue=false to have it; now the other way is not possible anymore).
In fact, Magnolia works like this with pretty much all other fields (not saving empty text fields, removing multi-value property in checkbox-groups, etc.).

If I read correctly, the issue was that the "surrounding trait" is not saved—not that it's missing a property.

Comment by Mikaël Geljić [ 21/Sep/17 ]

viet.nguyen,

whether or not it is optional, they would 90% expect a value from that

—That's a pretty wild assumption. We have the required and defaultValue params for that.

The current fix (forcing a defaultValue) is a behavior change; and cannot be applied to other fields for the simple reason that JCR does not allow null properties. See section 10.4.2.4 of the JCR spec. At any rate, such changes would have to be a conscious decision from PD/Architects.

Right now, the checkbox behavior is "altered" (all the more in a minor release), but the bug remains.
Think of it if people provide their own trait field, which does not store empty properties. His/her trait node should still be there.
Simple fields are not the culprit.

Comment by Roman Kovařík [ 25/Sep/17 ]

Reopened: issue remains for select boxes (possibly other fields) in the trait picker.

Comment by Milan Divilek [ 13/Oct/17 ]

Reopen:
This cause that UI test fails info.magnolia.eeintegrationtests.uitest.PersonalizationUITest.testStandardPersonalizationWorkflow (https://jenkins.magnolia-cms.com/view/Product_Team/job/ee_bundle-with-selenium_profile/2843/)

Problem is that creating page variant with no traits (only with segments) saves empty trait voter into jcr.. See screenshots


Comment by Roman Kovařík [ 13/Oct/17 ]

Could we fix the test? The behaviour seems correct.

Comment by Milan Divilek [ 13/Oct/17 ]

Problem is that this blocks whole variant.. Variant is never choose/render. It's not problem of test

Comment by Roman Kovařík [ 13/Oct/17 ]

Reverted, info.magnolia.personalization.ui.TraitAggregatorTransformer#removeProperty seems redundant with the fix for the RuleSet.
Marked as blocked until 5.4.x/5.5.7/5.6 releases are out.

Comment by Milan Divilek [ 24/Oct/17 ]

Reopen:
On branch 1.2.x - untouched trait is not stored

In segments app untouched trait is not stored for new segment.
Reproduce:
1. Open Segments app
2. Click "Add Segment" action
3. Switch to "Trait" tab and click "Pick Trait" and select "country" trait (don't touch it)
4. Save dialog
5. Select newly created segment and click "Edit segment" action
6. Switch to "Trait" tab... "country" trait is missing (Expected behaviour should be that it's there)
7. So click "Pick Trait" and select "country" trait (don't touch it)
8. Save dialog
8. Select segment and click "Edit segment" action
9. Switch to "Trait" tab... and "country" trait correctly is there

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