The checkbox 'Enable scheduled newsletter' doesn't work when you try to disble newsletter scheduling

Problem/Motivation

When we have the scheduled newsletter enabled and we want to disable it by unchecking the 'Enable scheduled newsletter' option, visually the option remains checked and in the 'simplenews_scheduler' table of the database the status of the 'activated' column never changes to 0.

This is because, when the data is collected from the database (line 39 of the simplenws_scheduler.module file) a condition is not set to select those rows that have the status of the 'activated' column to 1 and, therefore, it never goes through the else on line 50 of the same file.

Steps to reproduce

1. Check Activate scheduled newsletter option.
2. Click Save scheduler settings button.
3. Uncheck Activate scheduled newsletter option.
4. Click Save scheduler settings button.

Proposed resolution

In the data fetching (line 39 of the simplenews_scheduler.module file) add the condition to select the rows of the 'activated' column to 1.

With this we get that, if there are no elements, it passes by else of the condition of line 46 and the variable $checked is set to FALSE.

Hi einarulfhednar,

thank you for your merge request.

I think the solution is good but not perfect. If you add the condition, you cannot access the remaining config of the inactive schedule. If a user wants to disable the scheduling but keeping the interval, date values and so on, this is not possible. The user will loose the rest of the config.

What about setting the following patch? It sets checked based on the record and repairs the conditional invisible state of all following form fields, which was part of the code but broken.

My colleague goldfit found a bug in my patch. The submit button gets invisible, too. You cannot save a disabled scheduler. This is not part of my changes but the original code. I fixed that in the new patch.

Hey einarulfhednar,

I would be more then happy to fix this bug in 4.0.0-alpha3 but I need a review. Do you mind reviewing my latest patch from #5?

I can confirm this issue, it even happens when you enable scheduling.
The #5 patch works, thanks.
I am moving it to RTBC.

Okay, I will add the code from the patch to the issue fork, so that I can see what the pipeline says. If everything is fine, i will merge and create a new release.

Because I messed up the branch "3394957-the-checkbox-enable" i created a new one: simplenews-scheduler-3394957-b

Here is the corresponding MR: https://git.drupalcode.org/project/simplenews_scheduler/-/merge_requests/17

In this MR the patch leads to one failing PhpUnit test. In this case the failing test did what it should do: It showed me a bug. At the moment, however, I cannot yet explain why this occurs. Unfortunately, the time available to me to deal with this is limited.

Okay, the PhpUnit test fails, because the scheduler variable stays null, if the record variable is empty. But in the remaining code of the hook, the record variable is used several times. When I fix this bug, a follow bug appears.

The follow up bug is related to the following to core issues:

• https://www.drupal.org/project/drupal/issues/2419131 🐛 [PP-1] #states attribute does not work on #type datetime Postponed
• https://www.drupal.org/project/drupal/issues/3078334 🐛 Datetime and Datelist elements should render as fieldsets Needs work

The conditional visibility of the datetime form field 'stop_date' leads to an unwanted hiding of the parent container. In this case, the parent container is the container of the scheduler settings "$form['scheduler']['settings']". As long as the core bug isn't fixed, we need to wrap the datetime field widget into a container element. I will provide a fix for this in the merge request in the next days.

Going back to needs work.

I have added two commits to the merge request. The first fixes the problem with the unset scheduler variable, the second provides a work around for the "DateTime conditional visibility core bug" (see comment before this). The pipelines are happy, my local tests were also all successful. I move the issue back to "needs review"

I reviewed and tested the suggested code changes and have no complaints.