-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
ScheduledReminders - Refactor form to work at a standalone url #26884
ScheduledReminders - Refactor form to work at a standalone url #26884
Conversation
Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷 Introduction for new contributors
Quick links for reviewers |
* @var string[] | ||
*/ | ||
protected static $_paths = [ | ||
'add' => 'civicrm/admin/scheduleReminders/edit?reset=1&action=add', |
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.
If you look at the main listing page (/civicrm/admin/scheduleReminders?reset=1
), the Add Reminder
button still goes to the old URL (e.g. http://core-26884-62nfn.test-1.civicrm.org:8001/civicrm/admin/scheduleReminders?action=add&reset=1
). Shouldn't that change to match?
On the "Edit" screen on the autobuild test site, I see this warning - which I don't remember seeing before:
|
8941b23
to
e44a64f
Compare
OK, cool, that is better. I tested a bit with multi-tab usage (as in dev/core#4309). Each tab stores the data as you would expect. Woot. One last thing, though -- after you submit the form, where should you go? Before, it returned to the listing screen ( |
Thanks for the review @totten - I'm working on a metadata-driven solution for the redirects that can apply to every CRM_Admin_Form, not just this one, because I see all the decoupled forms now have this problem. |
Overview
One more form in an ongoing effort to decouple pages from forms. This one is for Scheduled Reminders.
Before
Form coupled with page. Exhibits bug in #26344
After
Form and page decoupled following standard pattern used for other admin forms. Bug fixed as a byproduct (https://lab.civicrm.org/dev/core/-/issues/4309).