Follow up to #3166226: Change timestamp and user data when updating / removing digests.
While testing for the 1.0.0 release I found that the timestamps are not updated when using the digest edit form. This is due to the form base using the generic set function which, of course, does not edit our timestamp.
Create a digest and save it. Note the value for "getNextSendDate".
Go back to the edit form for that digest and change the schedule so that the next send date should be different than before.
Note that the value of "getNextSendDate" has not changed.
It would not be smart to override the set function and check for "schedule". Developers will need to be aware that when using the generic functions they need to be careful to update time stamps. Ideas welcome for where exactly to document this.
This should only be a problem with the edit form (or other places where entities are generically edited) so add an update to the submission of the form.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3168584-7.patch | 2.76 KB | derekcresswell |
Comments
Comment #2
derekcresswell commentedHere is a failing test demonstrating this.
The digest has a timestamp set and is the updated through the form. The timestamp does not update.
Comment #3
derekcresswell commentedAnd here it is all fixed up. Feels good to have a strong base built up so issues like this are really only one line problems. : )
Comment #4
derekcresswell commentedComment #5
derekcresswell commentedHere is an alternate solution to this problem.
This overrides the
setfunction for the digest and checks for "schedule".While I don't think this is a great solution, I also don't like sticking this onto the form as before.
I main gripe I have with this is that for the
setSchedulefunction had a parameter to prevent this side effect. Adding this to the set function in my mind nullifies the usage of that parameter.Comment #6
derekcresswell commentedI've thought of another problem I have with the set function for this.
One of the nice bits of solving this on the form is that we are only setting this for existing digests. We don't want to have a bunch of time stamps left around for entities that were never saved.
This would mean we need to check if the entity is new before saving a time stamp. Not a hard check to add, but it should be placed within the
setNextSendDatefunction rather than just the set. And as I'm typing this I realise that since that is now needed I would be more fine with overriding the set function.I'll supply a new patch for that.
Comment #7
derekcresswell commentedImplemented a destructor to remove state from new digests. This lowers the amount of side effects caused by simply preventing new digests from adding the state.
No tests added for the destructor because it houses only internal functionality (despite being public). Perhaps a different solution could be found while updating tests. For now, I'd prefer keeping the public API small so there will be no public facing functions for that. We wouldn't want to limit digests to using state for this anyway.
This patch solves the problem of timestamps not updating with generic setting and fixes the issue of leaving info when deleting digests.
Comment #8
joshmillerComment #10
derekcresswell commented