Skip to content

Commit a230e5a

Browse files
nilmergnilmerg
authored
Merge pull request #122 from Icinga/fix-schedule-element
Enhance schedule element fields validation
2 parents 0bfed7d + af4f93a commit a230e5a

File tree

6 files changed

+99
-34
lines changed

6 files changed

+99
-34
lines changed

‎src/FormElement/ScheduleElement.php‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ public function setStart(DateTime $start): self
201201

202202
public function getValue($name = null, $default = null)
203203
{
204-
if ($name !== null) {
204+
if ($name !== null || ! $this->hasBeenValidated()) {
205205
return parent::getValue($name, $default);
206206
}
207207

@@ -469,6 +469,7 @@ protected function assemble()
469469
}
470470
} elseif ($this->hasCronExpression()) {
471471
$this->addElement('text', 'cron_expression', [
472+
'required' => true,
472473
'label' => $this->translate('Cron Expression'),
473474
'description' => $this->translate('Job cron Schedule'),
474475
'validators' => [
@@ -490,8 +491,28 @@ protected function assemble()
490491
new Recurrence('schedule-recurrences', [
491492
'id' => $this->protectId('schedule-recurrences'),
492493
'label' => $this->translate('Next occurrences'),
493-
'valid' => function (): bool {
494-
return $this->isValid();
494+
'validate' => function (): array {
495+
$isValid = $this->isValid();
496+
$reason = null;
497+
if (! $isValid && $this->getFrequency() === static::CUSTOM_EXPR) {
498+
if (! $this->getElement('interval')->isValid()) {
499+
$reason = current($this->getElement('interval')->getMessages());
500+
} else {
501+
$frequency = $this->getCustomFrequency();
502+
switch ($frequency) {
503+
case RRule::WEEKLY:
504+
$reason = current($this->weeklyField->getMessages());
505+
506+
break;
507+
default: // monthly
508+
$reason = current($this->monthlyFields->getMessages());
509+
510+
break;
511+
}
512+
}
513+
}
514+
515+
return [$isValid, $reason];
495516
},
496517
'frequency' => function (): Frequency {
497518
if ($this->getFrequency() === static::CUSTOM_EXPR) {
@@ -564,7 +585,7 @@ private function assembleCommonElements(): void
564585
public function prepareMultipartUpdate(RequestInterface $request): array
565586
{
566587
$autoSubmittedBy = $request->getHeader('X-Icinga-AutoSubmittedBy');
567-
$pattern = '/\[(weekly-fields|monthly-fields|annually-fields)]\[(ordinal|interval|month|day(\d+)?|[A-Z]{2})]$/';
588+
$pattern = '/\[(weekly-fields|monthly-fields|annually-fields)]\[(ordinal|month|day(\d+)?|[A-Z]{2})]$/';
568589

569590
$partUpdates = [];
570591
if (
@@ -573,8 +594,11 @@ public function prepareMultipartUpdate(RequestInterface $request): array
573594
&& (
574595
preg_match('/\[(start|end)]$/', $autoSubmittedBy[0], $matches)
575596
|| preg_match($pattern, $autoSubmittedBy[0])
597+
|| preg_match('/\[interval]/', $autoSubmittedBy[0])
576598
)
577599
) {
600+
$this->ensureAssembled();
601+
578602
$partUpdates[] = $this->getElement('schedule-recurrences');
579603
if (
580604
$this->getFrequency() === static::CUSTOM_EXPR

‎src/FormElement/ScheduleElement/Common/FieldsUtils.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,9 @@ public function loadRRule(RRule $rule): array
8585
$values = [];
8686
$isMonthly = $rule->getFrequency() === RRule::MONTHLY;
8787
if ($isMonthly && (! empty($rule->getByMonthDay()) || empty($rule->getByDay()))) {
88-
foreach ($rule->getByMonthDay() ?? [] as $value) {
89-
$values["day$value"] = 'y';
88+
$monthDays = $rule->getByMonthDay() ?? [];
89+
foreach (range(1, $this->availableFields) as $value) {
90+
$values["day$value"] = in_array((string) $value, $monthDays, true) ? 'y' : 'n';
9091
}
9192

9293
$values['runsOn'] = MonthlyFields::RUNS_EACH;

‎src/FormElement/ScheduleElement/MonthlyFields.php‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@
55
use ipl\Html\Attributes;
66
use ipl\Html\FormElement\FieldsetElement;
77
use ipl\Html\HtmlElement;
8+
use ipl\Validator\CallbackValidator;
89
use ipl\Validator\InArrayValidator;
10+
use ipl\Validator\ValidatorChain;
911
use ipl\Web\FormElement\ScheduleElement\Common\FieldsProtector;
1012
use ipl\Web\FormElement\ScheduleElement\Common\FieldsUtils;
1113

@@ -103,15 +105,13 @@ protected function assemble()
103105
$listItems->getAttributes()->add('class', 'disabled');
104106
}
105107

106-
$foundCheckedDay = false;
107108
foreach (range(1, $this->availableFields) as $day) {
108109
$checkbox = $this->createElement('checkbox', "day$day", [
109110
'class' => ['autosubmit', 'sr-only'],
110-
'value' => $this->getPopulatedValue("day$day", 'n')
111+
'value' => $day === $this->default && $runsOn === static::RUNS_EACH
111112
]);
112113
$this->registerElement($checkbox);
113114

114-
$foundCheckedDay = $foundCheckedDay || $checkbox->isChecked();
115115
$htmlId = $this->protectId("day$day");
116116
$checkbox->getAttributes()->set('id', $htmlId);
117117

@@ -120,10 +120,6 @@ protected function assemble()
120120
$listItems->addHtml($listItem);
121121
}
122122

123-
if (! $foundCheckedDay) {
124-
$this->getElement("day{$this->default}")->setChecked(true);
125-
}
126-
127123
$monthlyWrapper = HtmlElement::create('div', ['class' => 'monthly']);
128124
$runsEach = $this->getElement('runsOn');
129125
$runsEach->prependWrapper($monthlyWrapper);
@@ -166,4 +162,30 @@ protected function registerAttributeCallbacks(Attributes $attributes)
166162
->registerAttributeCallback('availableFields', null, [$this, 'setAvailableFields'])
167163
->registerAttributeCallback('protector', null, [$this, 'setIdProtector']);
168164
}
165+
166+
protected function addDefaultValidators(ValidatorChain $chain): void
167+
{
168+
$chain->add(
169+
new CallbackValidator(function ($_, CallbackValidator $validator): bool {
170+
if ($this->getValue('runsOn', static::RUNS_EACH) !== static::RUNS_EACH) {
171+
return true;
172+
}
173+
174+
$valid = false;
175+
foreach (range(1, $this->availableFields) as $day) {
176+
if ($this->getValue("day$day") === 'y') {
177+
$valid = true;
178+
179+
break;
180+
}
181+
}
182+
183+
if (! $valid) {
184+
$validator->addMessage($this->translate('You must select at least one of these days'));
185+
}
186+
187+
return $valid;
188+
})
189+
);
190+
}
169191
}

‎src/FormElement/ScheduleElement/Recurrence.php‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ class Recurrence extends BaseFormElement
2323
protected $frequencyCallback;
2424

2525
/** @var callable A validation callback for the schedule element */
26-
protected $isValidCallback;
26+
protected $validateCallback;
2727

2828
/**
2929
* Set a validation callback that will be called when assembling this element
@@ -34,7 +34,7 @@ class Recurrence extends BaseFormElement
3434
*/
3535
public function setValid(callable $callback): self
3636
{
37-
$this->isValidCallback = $callback;
37+
$this->validateCallback = $callback;
3838

3939
return $this;
4040
}
@@ -55,8 +55,11 @@ public function setFrequency(callable $callback): self
5555

5656
protected function assemble()
5757
{
58-
$isValid = ($this->isValidCallback)();
58+
list($isValid, $reason) = ($this->validateCallback)();
5959
if (! $isValid) {
60+
// Render why we can't generate the recurrences
61+
$this->addHtml(Text::create($reason));
62+
6063
return;
6164
}
6265

@@ -81,6 +84,6 @@ protected function registerAttributeCallbacks(Attributes $attributes)
8184

8285
$attributes
8386
->registerAttributeCallback('frequency', null, [$this, 'setFrequency'])
84-
->registerAttributeCallback('valid', null, [$this, 'setValid']);
87+
->registerAttributeCallback('validate', null, [$this, 'setValid']);
8588
}
8689
}

‎src/FormElement/ScheduleElement/WeeklyFields.php‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
use ipl\Html\Attributes;
77
use ipl\Html\FormElement\FieldsetElement;
88
use ipl\Html\HtmlElement;
9+
use ipl\Validator\CallbackValidator;
10+
use ipl\Validator\ValidatorChain;
911
use ipl\Web\FormElement\ScheduleElement\Common\FieldsProtector;
1012

1113
class WeeklyFields extends FieldsetElement
@@ -76,20 +78,15 @@ public function getSelectedWeekDays(): array
7678
/**
7779
* Transform the given weekdays into key=>value array that can be populated
7880
*
79-
* @param array $days
81+
* @param array $weekdays
8082
*
8183
* @return array
8284
*/
83-
public function loadWeekDays(array $days): array
85+
public function loadWeekDays(array $weekdays): array
8486
{
8587
$values = [];
86-
foreach ($days as $day) {
87-
$weekDays = strtoupper($day);
88-
if (! isset($this->weekdays[$weekDays])) {
89-
throw new InvalidArgumentException(sprintf('Invalid weekday provided: %s', $day));
90-
}
91-
92-
$values[$weekDays] = 'y';
88+
foreach ($this->weekdays as $weekday => $_) {
89+
$values[$weekday] = in_array($weekday, $weekdays, true) ? 'y' : 'n';
9390
}
9491

9592
return $values;
@@ -102,15 +99,13 @@ protected function assemble()
10299
$fieldsWrapper = HtmlElement::create('div', ['class' => 'weekly']);
103100
$listItems = HtmlElement::create('ul', ['class' => ['schedule-element-fields', 'multiple-fields']]);
104101

105-
$foundCheckedDay = false;
106102
foreach ($this->weekdays as $day => $value) {
107103
$checkbox = $this->createElement('checkbox', $day, [
108104
'class' => ['autosubmit', 'sr-only'],
109-
'value' => $this->getPopulatedValue($day, 'n')
105+
'value' => $day === $this->default
110106
]);
111107
$this->registerElement($checkbox);
112108

113-
$foundCheckedDay = $foundCheckedDay || $checkbox->isChecked();
114109
$htmlId = $this->protectId("weekday-$day");
115110
$checkbox->getAttributes()->set('id', $htmlId);
116111

@@ -119,10 +114,6 @@ protected function assemble()
119114
$listItems->addHtml($listItem);
120115
}
121116

122-
if (! $foundCheckedDay) {
123-
$this->getElement($this->default)->setChecked(true);
124-
}
125-
126117
$fieldsWrapper->addHtml($listItems);
127118
$this->addHtml($fieldsWrapper);
128119
}
@@ -135,4 +126,26 @@ protected function registerAttributeCallbacks(Attributes $attributes)
135126
->registerAttributeCallback('default', null, [$this, 'setDefault'])
136127
->registerAttributeCallback('protector', null, [$this, 'setIdProtector']);
137128
}
129+
130+
protected function addDefaultValidators(ValidatorChain $chain): void
131+
{
132+
$chain->add(
133+
new CallbackValidator(function ($_, CallbackValidator $validator): bool {
134+
$valid = false;
135+
foreach ($this->weekdays as $weekday => $_) {
136+
if ($this->getValue($weekday) === 'y') {
137+
$valid = true;
138+
139+
break;
140+
}
141+
}
142+
143+
if (! $valid) {
144+
$validator->addMessage($this->translate('You must select at least one of these weekdays'));
145+
}
146+
147+
return $valid;
148+
})
149+
);
150+
}
138151
}

‎tests/ScheduleElementTest.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,11 @@ protected function setUp(): void
2020
protected function assembleElement(array $options): ScheduleElement
2121
{
2222
$element = new ScheduleElement('test', $options);
23+
// ScheduleElement#getValue won't return an instance of Frequency if it's not validated
24+
$element->ensureAssembled()->validate();
2325

2426
// Remove the recurrences preview. It randomizes the HTML and isn't subject to test
25-
if ($element->ensureAssembled()->hasElement('schedule-recurrences')) {
27+
if ($element->hasElement('schedule-recurrences')) {
2628
$element->remove($element->getElement('schedule-recurrences'));
2729
}
2830

0 commit comments

Comments
 (0)