[#37536] - [5.4] Task Scheduler require custom cron execution rules to be filled out
- Pending
- Medium
- Build: 4.2-dev
- # 37536
- Diff
- magnussinger:i37313
User tests: Successful: Unsuccessful:
Pull Request for Issue #37313
Summary of Changes
When you create a new scheduled task and select custom execution rules, and you don't specify a execution time, you can't save it now
Testing Instructions
- Create Demo Task - Sleep
Select Cron Expression Advanced as Execution Rule
Do not select all values in area Cron match
Try to save, Joomla will show you that you still have empty fields - Change execution rule to something else than Cron Expression Advanced and check if that is still working as before.
Actual result BEFORE applying this Pull Request
- You were able to click save, but then ran into an error
Expected result AFTER applying this Pull Request
- You see that there are required fields left and you cannot save
- Other execution rules than Cron Expression Advanced are still working.
Documentation Changes Required
None
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration |
| Labels |
Added:
?
|
||
| Title |
|
||||||
@richard67 thanks for the hint, I'll have a look at it
See: #37537 (comment)
| Category | Administration | ⇒ | Administration Repository NPM Change JavaScript |
@bembelimen @richard67 I tried to control it now via JS, please let me know if this is okay
| Labels |
Added:
NPM Resource Changed
|
||
@dgrammatiko Could you do a quick review on the JS in this PR? To me it looks good, but I am not really a JS expert. Thanks in advance.
@MagnusSinger Could you extend your testing instructions by a test if other execution rules than advanced cron expressions don't require the cron parameters (i.e. mainly a test for your last change)? Thanks in advance. In general it's important that testing instructions don't cover only the test for the fixed issue but also the test that other behaviour of the changed code is not broken.
We need for sure also a server side validation for this (e.g. in the models validate method)
This pull requests has been automatically converted to the PSR-12 coding standard.
This pull request has been automatically rebased to 4.3-dev.
| Labels |
Added:
?
bug
|
||
| Labels |
Added:
PR-4.3-dev
|
||
This pull request has been automatically rebased to 4.4-dev.
| Title |
|
||||||
This issue still persists even in version 5.2. The "Cron Expression (Advanced)" option remains virtually unusable that way.
| Title |
|
||||||
@magnussinger Can you please fix the conflicts?
This pull request has been automatically rebased to 5.3-dev.
| Title |
|
||||||
This pull request has been automatically rebased to 5.4-dev.
| Title |
|
||||||
| Labels |
Added:
PR-5.4-dev
Removed: ? ? PR-4.3-dev |
||
I've allowed myself to fix the merge conflict coming from the removal of the es5 scripts in the base branch.
@magnussinger Could you check @n3t 's review comment? Is there something to be changed in this PR?
There is no function Array.contains in Javascript, only Array.includes, therefore instead of
['add', 'remove'].contains(process) should be ['add', 'remove'].includes(process)...
I tried to reproduce the problem, but the error from the issue #37313 was no more seen. If the cron fields are not selected there is a clear error message without this PR:
@astridx Could you please try by your own with 5.4?
With this PR there are the individual error messages:
If everything works, shall we use standard error handling to standardise the user experience and reduce the code, and close this PR?
@MagnusSinger My comment from another, closed PR for the same issue also applies here, see #37350 (comment) .
Also the testing instructions should include a test that nothingness broken for other kinds of execution rules that the advanced cron expression .