-
Notifications
You must be signed in to change notification settings - Fork 233
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
✨ Allow AttributeSet in XML sample file #654
base: develop
Are you sure you want to change the base?
Conversation
In XML Data Loader, manage AttributeSet class and its children to be imported , in sample XML files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good!
IMHO wouldn't it be better to remove the elseif
section and just add the $value = $oAttDef->MakeRealValue($value, $oTargetObj);
part in the existing else
?
Hello Guy, Thanks for the PR and for the unit test! Seems like a good idea. |
Hello, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technical review: The actual fix would be to fix the general else case and remove the TODO:
- Remove the existing
elseif
forAttributeTagSet
(and yourelseif
) - In the final
else
- Replace
$res = $oTargetObj->CheckValue($sAttCode, $value);
- With
$res = $oTargetObj->CheckValue($sAttCode, $oAttDef->MakeRealValue($value, $oTargetObj));
- Replace
Accepted in functional review. Thanks @GurneyHallack for the contribution, we'll make the last edits. |
Hello, If we change the "else", ideally, we would need to check for every type of Attributes possibles, and with empty or not empty values. In the XMLDataLoaderTest.php, there is :
|
In XML Data Loader, manage AttributeSet class and its children to be imported , in sample XML files.
Base information
Symptom (bug) / Objective (enhancement)
In the setup (unattended or not), it's not possible to load extensions which contains sample XML file which contain AttributeEnumSet, AttributeClassAttCodeSet, AttributeQueryAttCodeSet or AttributeTagSet attribute, with values (not empty).
It generates an error :
Reproduction procedure (bug)
<TriggerOnObjectUpdate alias="TriggerOnObjectUpdate" id="4"> <description>Contact updated</description> <action_list></action_list> <context>GUI:Console</context> <complement>class restriction: Contact</complement> <subscription_policy>allow_no_channel</subscription_policy> <target_class>Contact</target_class> <filter>SELECT `Person` FROM Person AS `Person` WHERE ((`status` = 'active') AND ((`org_id` = :current_contact->org_id) OR (`org_id` = :this->org_id)))</filter> <target_attcodes>email</target_attcodes> <finalclass>TriggerOnObjectUpdate</finalclass> <friendlyname>Contact updated</friendlyname> </TriggerOnObjectUpdate>
Cause (bug)
Function IsNull of AttributeSet assumes that $proposedValue is a ormSet. However, in XMLDataloader, we send a string.
Thus, the function Count, which doesn't exist in string, generates an exception.
Proposed solution (bug and enhancement)
I added in XMLDataLoader a elseif to manage AttributeSet class and its children.
I didn't change the function IsNull of the AttributeSet, to check if the $proposedValue is of class ormSet.
Checklist before requesting a review
Checklist of things to do before PR is ready to merge