Skip to content

Commit

Permalink
Fully support comma-separated constraint lists in Form Customization (m…
Browse files Browse the repository at this point in the history 
…odxcms#16555)

### What does it do?
In addition to correctly handling TV customization via FC, the proposed
changes:
1. Optimize the FC Set Update processor code
2. Add validation to prevent incomplete constraint specification
(_e.g._, user fills in the Constraint Field but no the Constraint, or
vice versa) in the Set editing panel
3. Log a warning message if an incomplete constraint spec is saved from
the grid. (Here, we don't want to trigger an error [preventing save]
like we do in the stand alone editing panel because you can not recover
from an error in an editor grid set to autosave.) This may point to a
need for an intermediary validation state to be created where, like
addError/addFieldError, you could trigger an addWarning/addFieldWarning
on a field to alert users to a problem but not halt the save process.
4. Provides basic cleanup of user input for the three primary FC Set
text fields (trim fields and help ensure proper constraint format)
5. Makes a tiny change to the Update controller to fix a deprecation
warning issue re the usage of htmlspecialchars
6. Separate commit cleans up code quality/formatting issues in the php
files

### Why is it needed?
TV re-labelling, default value alteration, etc is not currently working
when providing multiple constraints. Also, the UI in this area is
somewhat lacking in terms of validation and input handling (not cleaning
user data to any degree).

### How to test
1. Create a one or more TVs
2. Create an FC Profile and Set
3. Experiment with all FC operations (moving and re-labeling fields,
creating and renaming regions, setting default values, etc), verifying
all works as expected for both standard fields and TVs
4. Try saving your Set with incomplete constraint information to verify
validation works and messaging is clear

### Related issue(s)/PR(s)
This is a more comprehensive treatment related to a currently-open PR
for 2.x (modxcms#16417)

---------

Co-authored-by: Jason Coward <[email protected]>
  • Loading branch information
@smg6511 @opengeek
smg6511 and opengeek authored Sep 4, 2024
1 parent 9a3f82c commit 4ab51e3
Show file tree
Hide file tree
Showing 12 changed files with 403 additions and 255 deletions.
4 changes: 4 additions & 0 deletions core/lexicon/en/formcustomization.inc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
$_lang['constraint_desc'] = 'Optional. The value of the Constraint Field that should be checked against.';
$_lang['constraint_field'] = 'Constraint Field';
$_lang['constraint_field_desc'] = 'Optional. The field by which this constraint should be applied.';
$_lang['constraint_incomplete_constraint_err'] = 'To apply constraints, the Constraint must be specified for this rule to work properly.';
$_lang['constraint_incomplete_constraint_warn'] = 'Set #%d in the Form Customization profile named “%s” may not work as expected because a Constraint Field was set but the Constraint is missing.';
$_lang['constraint_incomplete_field_err'] = 'To apply constraints, the Constraint Field must be specified for this rule to work properly.';
$_lang['constraint_incomplete_field_warn'] = 'Set #%d in the Form Customization profile named “%s” may not work as expected because a Constraint was set but the Constraint Field is missing.';
$_lang['containing_panel'] = 'Containing Panel';
$_lang['containing_panel_desc'] = 'The ID of the containing Form Panel the field is in. This is sometimes necessary for certain rules, so that the system can know what form or panel the field is in.';
$_lang['deactivate'] = 'Deactivate';
Expand Down
35 changes: 34 additions & 1 deletion core/src/Revolution/Processors/Security/Forms/Set/Create.php
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

/*
* This file is part of MODX Revolution.
*
Expand All @@ -25,16 +26,48 @@ class Create extends CreateProcessor
public $permission = 'customize_forms';
public $objectType = 'set';

protected $fcSetId;
protected $fcSetAction;
protected $fcSetConstraintClass = modResource::class;
protected $fcSetConstraintField;
protected $fcSetConstraint;

/**
* @return bool
*/
public function beforeSet()
{
$this->fcSetId = $this->object->get('id');
$this->fcSetAction = $this->object->get('action');
$this->fcSetConstraintField = trim($this->getProperty('constraint_field', ''));
$this->fcSetConstraint = trim($this->getProperty('constraint', ''));

$this->setProperty('constraint_field', $this->fcSetConstraintField);
$this->setProperty('constraint', $this->fcSetConstraint);
$this->setProperty('description', trim($this->getProperty('description', '')));

return parent::beforeSet();
}

/**
* @return bool
*/
public function beforeSave()
{
$this->object->set('constraint_class', modResource::class);
$this->object->set('constraint_class', $this->fcSetConstraintClass);
$actionId = $this->getProperty('action_id');
if ($actionId !== null) {
$this->object->set('action', $actionId);
}
$hasConstraintField = !empty($this->fcSetConstraintField);
if (!$hasConstraintField xor (empty($this->fcSetConstraint) && $this->fcSetConstraint !== 0)) {
if (!$hasConstraintField) {
$this->addFieldError('constraint_field', $this->modx->lexicon('constraint_incomplete_field_err'));
} else {
$this->addFieldError('constraint', $this->modx->lexicon('constraint_incomplete_constraint_err'));
}
}

return parent::beforeSave();
}
}
187 changes: 62 additions & 125 deletions core/src/Revolution/Processors/Security/Forms/Set/Update.php
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

/*
* This file is part of MODX Revolution.
*
Expand Down Expand Up @@ -35,12 +36,27 @@ class Update extends UpdateProcessor
/** @var array $newRules */
public $newRules = [];

protected $fcSetId;
protected $fcSetAction;
protected $fcSetConstraintClass = modResource::class;
protected $fcSetConstraintField;
protected $fcSetConstraint;

/**
* @return bool
*/
public function beforeSet()
{
$this->fcSetId = $this->object->get('id');
$this->fcSetAction = $this->object->get('action');
$this->fcSetConstraintField = trim($this->getProperty('constraint_field', ''));
$this->fcSetConstraint = trim($this->getProperty('constraint', ''));

$this->setProperty('constraint_field', $this->fcSetConstraintField);
$this->setProperty('constraint', $this->fcSetConstraint);
$this->setProperty('description', trim($this->getProperty('description', '')));
$this->setCheckbox('active');

return parent::beforeSet();
}

Expand All @@ -49,11 +65,19 @@ public function beforeSet()
*/
public function beforeSave()
{
$this->object->set('constraint_class', modResource::class);
$this->object->set('constraint_class', $this->fcSetConstraintClass);
$actionId = $this->getProperty('action_id');
if ($actionId !== null) {
$this->object->set('action', $actionId);
}
$hasConstraintField = !empty($this->fcSetConstraintField);
if (!$hasConstraintField xor (empty($this->fcSetConstraint) && $this->fcSetConstraint !== 0)) {
if (!$hasConstraintField) {
$this->addFieldError('constraint_field', $this->modx->lexicon('constraint_incomplete_field_err'));
} else {
$this->addFieldError('constraint', $this->modx->lexicon('constraint_incomplete_constraint_err'));
}
}

return parent::beforeSave();
}
Expand Down Expand Up @@ -101,58 +125,25 @@ public function setFieldRules()
$fields = is_array($fields) ? $fields : $this->modx->fromJSON($fields);

foreach ($fields as $field) {
$targetName = $field['name'];
if (empty($field['visible'])) {
/** @var modActionDom $rule */
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', $field['name']);
$rule->set('container', 'modx-panel-resource');
$rule = $this->createRule($targetName);
$rule->set('rule', 'fieldVisible');
$rule->set('value', 0);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 5);
$this->newRules[] = $rule;
}
if (!empty($field['label'])) {
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', $field['name']);
$rule->set('container', 'modx-panel-resource');
$rule = $this->createRule($targetName);
$rule->set('rule', 'fieldTitle');
$rule->set('value', $field['label']);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 4);
$this->newRules[] = $rule;
}
if (isset($field['default_value']) && $field['default_value'] !== '') {
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', $field['name']);
$rule->set('container', 'modx-panel-resource');
$rule = $this->createRule($targetName);
$rule->set('rule', 'fieldDefault');
$rule->set('value', $field['default_value']);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 0);
$this->newRules[] = $rule;
}
Expand Down Expand Up @@ -184,60 +175,27 @@ public function setTabRules()
'name' => $tab['name'],
'type' => 'tab',
]);
$targetName = $tab['name'];
/* if creating a new tab */
if (empty($tabField) && !empty($tab['visible'])) {
/** @var modActionDom $rule */
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', $tab['name']);
$rule->set('container', 'modx-resource-tabs');
$rule = $this->createRule($targetName, 'modx-resource-tabs');
$rule->set('rule', 'tabNew');
$rule->set('value', $tab['label']);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 1);
$this->newRules[] = $rule;
} else {
/* otherwise editing an existing one */
if (empty($tab['visible'])) {
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', $tab['name']);
$rule->set('container', 'modx-resource-tabs');
$rule = $this->createRule($targetName, 'modx-resource-tabs');
$rule->set('rule', 'tabVisible');
$rule->set('value', 0);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 2);
$this->newRules[] = $rule;
}
if (!empty($tab['label'])) {
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', $tab['name']);
$rule->set('container', 'modx-resource-tabs');
$rule = $this->createRule($targetName, 'modx-resource-tabs');
$rule->set('rule', 'tabTitle');
$rule->set('value', $tab['label']);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 3);
$this->newRules[] = $rule;
}
Expand All @@ -264,77 +222,32 @@ public function setTVRules()
if ($tv === null) {
continue;
}

$targetName = 'tv' . $tvData['id'];
if (empty($tvData['visible'])) {
/** @var modActionDom $rule */
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', 'tv' . $tv->get('id'));
$rule->set('container', 'modx-panel-resource');
$rule = $this->createRule($targetName);
$rule->set('rule', 'tvVisible');
$rule->set('value', 0);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 12);
$this->newRules[] = $rule;
}
if (!empty($tvData['label'])) {
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', 'tv' . $tv->get('id'));
$rule->set('container', 'modx-panel-resource');
$rule = $this->createRule($targetName);
$rule->set('rule', 'tvTitle');
$rule->set('value', $tvData['label']);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 11);
$this->newRules[] = $rule;
}
if ($tv->get('default_text') !== $tvData['default_value']) {
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', 'tv' . $tv->get('id'));
$rule->set('container', 'modx-panel-resource');
$rule = $this->createRule($targetName);
$rule->set('rule', 'tvDefault');
$rule->set('value', $tvData['default_value']);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
$rule->set('rank', 10);
$this->newRules[] = $rule;
}
if (!empty($tvData['tab']) && $tvData['tab'] !== 'modx-panel-resource-tv') {
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
$rule->set('name', 'tv' . $tv->get('id'));
$rule->set('container', 'modx-panel-resource');
$rule = $this->createRule($targetName);
$rule->set('rule', 'tvMove');
$rule->set('value', $tvData['tab']);
$rule->set('constraint_class', $this->object->get('constraint_class'));
$rule->set('constraint_field', $this->object->get('constraint_field'));
$rule->set('constraint', $this->object->get('constraint'));
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
/* add 20 to rank to make sure happens after tab create */
$rank = 20 + ((int)$tvData['rank']);
$rule->set('rank', $rank);
Expand All @@ -354,4 +267,28 @@ public function saveNewRules()
$newRule->save();
}
}

/**
* Creates initial customization rule specification, to be further defined each time this method is called
* @param string $targetName An identifier for the field, tab, or TV to be transformed
* @param string $container The id of the parent object (ExtJS config) that contains the field, tab,
* or TV to be transformed
* @return modActionDom
*/
protected function createRule(string $targetName, string $container = 'modx-panel-resource'): modActionDom
{
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->fcSetId);
$rule->set('action', $this->fcSetAction);
$rule->set('name', $targetName);
$rule->set('container', $container);
$rule->set('constraint_class', $this->fcSetConstraintClass);
$rule->set('constraint_field', $this->fcSetConstraintField);
$rule->set('constraint', $this->fcSetConstraint);
$rule->set('active', true);
if ($this->object->get('action') === 'resource/create') {
$rule->set('for_parent', true);
}
return $rule;
}
}
Loading

0 comments on commit 4ab51e3

Please sign in to comment.