Skip to content
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

dev/core#2587 Ignore modified_date when checking for changes during mailing auto-save #20561

Merged
merged 1 commit into from Jan 7, 2022

Conversation

rthouvenin
Copy link
Contributor

Overview

Prevents double auto-save at each modification of a mailing in the mailing editor.
https://lab.civicrm.org/dev/core/-/issues/2587

Before

Whenever a mailing or A/B mailing is modified in the editor, the auto-save is triggered twice: once because the mailing was modified, once because after the first save the modified date of the local object does not match the one of the server response (because the server overwrote its value).

After

The modified date is ignored in the comparison for the auto-save, so that the second unnecessary save does not happen.

Technical Details

The suggestion of the issue on how to implement the fix does not work because the model is not a single object, but a list of objects (mailing(s), optional ab mailing, attachments). The autoSave controller is generic and is agnostic of the structure of the model, so finding and ignoring the modified date could not happen there. This is why instead I chose to edit the model before giving it to the autoSave controller. As far as I can see this affects only the autoSave.

Comments

When testing the fix, you may notice that the autoSave still happens twice, but only for the first time it is triggered. I found out this is another unrelated bug, due to the schedule date being set to empty string or null depending on whether it comes from initialisation or server response. I didn't chase this bug further.

@civibot
Copy link

civibot bot commented Jun 9, 2021

(Standard links)

@civibot civibot bot added the master label Jun 9, 2021
@eileenmcnaughton
Copy link
Contributor

@colemanw are you able to take a look at this one?

@demeritcowboy
Copy link
Contributor

This seems to run ok and I can see it prevents calling save twice and I don't see any side effects. Going to put merge ready.

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Jan 4, 2022
@demeritcowboy demeritcowboy merged commit dedb7e7 into civicrm:master Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
3 participants