Prevent data loss on save (entries and sections) #2659

nitriques · 2017-04-21T15:42:54Z

We add a timestamp check before saving records.

When saving a section, if the data changes between the render of the
page and the form post, it can lead to data loss if a field was added.
The SQL will crash because it assumes the fields did not change.

When saving and entry, the SQL was ok, but still, it can lead to data
loss.

At both places, the fix provided here makes sure we are editing the
lastest version of the record.

cc @brendo @michael-e

brendo

So I'm assuming the position is that this protection will not be enabled for entries saved from Event's? This is purely a backend only feature to help in scenarios where there are multiple authors?

brendo · 2017-04-22T04:59:57Z

symphony/lib/toolkit/class.timestampvalidator.php

+     */
+    public function __construct($table)
+    {
+        assert($table, 'Table cannot be falsy');


Shouldn't really rely on this for production...

Why ? You would prefer to throw ?

The behaviour of assert depends on php configuration values. The idea is usually that development code will throw errors, but production instances optimise away the calls.

I'd prefer regular throwing in this scenario.

Fixed in d922795. Thanks!

brendo · 2017-04-22T05:00:14Z

symphony/lib/toolkit/class.sectionmanager.php

+        $defaults = array();
+        $defaults['creation_date'] = $defaults['modification_date'] = DateTimeObj::get('Y-m-d H:i:s');
+        $defaults['creation_date_gmt'] = $defaults['modification_date_gmt'] = DateTimeObj::getGMT('Y-m-d H:i:s');
+        $defaults['author_id'] = '1';


Noticed below it's 1, any reason for a string here?

Because it's a copy of this line https://github.com/symphonycms/symphony-2/blob/2.7.x/symphony/lib/toolkit/class.entry.php#L105

Should we change both ?

Yeah I think both should become integers.

Fixed in d1d7ddf

jonmifsud · 2017-04-23T08:47:39Z

@nitriques nice touch was looking at this from an extension perspective to provide a locking functionality showing that another user is editing.

From what I can see however (code inspection) there is no way someone can confirm an override. So if I had two tabs open and/or there was another user who saved. But I actually want to save my version what does actually happen?

Secondly if I save and get the error, I'm presuming the post data would be pre-populated as per existing functionality. If so wouldn't saving again have the right time-stamp or is that also in a way maintained through post? Might have missed something as I didn't try it out.

nitriques · 2017-04-25T16:47:39Z

So I'm assuming the position is that this protection will not be enabled for entries saved from Event's? This is purely a backend only feature to help in scenarios where there are multiple authors?

@brendo Indeed. Right now, the validation is in the publish pages. I did not wanted to break things with this feature. But any entry updated via the frontend will get a new mod_date, so the validation will fail for authors.

But I actually want to save my version what does actually happen?

Would have to start from scratch. I would hope to have time to do some diffs and try to resolve conflicts, but hey. that's hard.

Secondly if I save and get the error, I'm presuming the post data would be pre-populated as per existing functionality.

Exactly. Right now, the save will NEVER work. You have to refresh this page and start from scratch.

This is why it's a PR ;) Let's debate.

michael-e · 2017-04-26T12:57:10Z

I understand the reasoning, but I still try to understand the solution.

Does this mean that still two people can open the same entry, but only the first save will be successful? This wouldn't be any better than "the last save wins" (which is what we have now).

Also, I think that any "lock" needs the possibilty to "override" the lock.

Shouldn't a proper solution prevent access to an entry that is edited currently (by another author)? (Also this would need the possibility to override the lock.)

nitriques · 2017-04-26T21:03:43Z

Does this mean that still two people can open the same entry, but only the first save will be successful?

Yes.

This wouldn't be any better than "the last save wins"

I do not agree. We had calls from client because of this, and it took the activity tracker to prove to the client that this was happening. From a UX point of view, it's better IMHO.

I think that the current error message could be improve, to let the user know more about what happened, like who did the last save and on what time.

michael-e · 2017-04-27T12:42:26Z

As I said before, I think that a proper solution should in advance prevent access to an entry that is currently edited (by another author). With your solution, authors will ask "Why have you allowed me to edit an entry if I am not allowed to save my work?". There is no good answer to that.

One way or another, there should be the possibility to override the "lock", i.e. save nevertheless.

jonmifsud · 2017-04-27T13:21:37Z

Agreed re override. That's a must have. Might also debate if this is something for the core to handle.. at least I would be happy with this working as you suggested + another flag override entry which would skip/ignore the check. Could be a checkbox in the UI - but equally used by other API's to update entries regardless. So if I get the error modal I don't need to re-start but I can say Ignore the other dude's changes I want mine.

…

On Thu, Apr 27, 2017 at 2:42 PM, michael-e ***@***.***> wrote: As I said before, I think that a proper solution should in advance prevent access to an entry that is currently edited (by another author). With your solution, authors will ask "Why have you allowed me to edit an entry if I am not allowed to save my work?". There is no good answer to that. One way or another, there should be the possibility to override the "lock", i.e. save nevertheless. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2659 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0ef8BbN3Vj8nn3bHEmA9qh2Mv5qpPwks5r0I0zgaJpZM4NEd69> .

nitriques · 2017-04-27T14:41:10Z

I think that a proper solution should in advance prevent access to an entry that is currently edited

That's not always possible. Let's say I open the edit page and go make a coffee. I come back, edit and then press save: only then, do the system know that it's trying to edit based on old records.

One way or another, there should be the possibility to override the "lock", i.e. save nevertheless.

Ok this seems to make consensus, I'll do it. Thanks guys!

nitriques · 2017-04-27T17:04:20Z

Why are we moving away from Symphony::Database() in favor of MySQL

Right now, cleanValue is static, so it should be called with the class name. This needs to change.

As for the UI, I've come up with this:

michael-e · 2017-04-27T17:13:47Z

"The action requested…" sounds complicated. What about "Saving failed. The entry has been changed by Test on…"?

And instead of "Override? Please understand…" I would prefer "Overwrite these changes?".

This should be decided by a native speaker. @brendo, ah, no, he's Australian… Hmmm… :-)

michael-e · 2017-04-27T17:14:48Z

Right now, cleanValue is static, so it should be called with the class name. This needs to change.

I understand. Thanks.

nitriques · 2017-04-27T18:39:56Z

This should be decided by a native speaker. @brendo, ah, no, he's Australian… Hmmm… :-)

Hahaha. But yeah, I need help with that haha

nitriques · 2017-04-27T18:40:30Z

I've got override working, and I'm back with the non-existing table problem hahaha

nitriques · 2017-04-27T20:28:05Z

1914abc is the fix I've meant to not do, by, instead, creating the timestamp validation. But with overriding in place, it still needed to be done. I hope the fix does not suck as much as I think it sucks, but hey, it is the only one I was able to do it.

@brendo if you could get your blessing regarding the wording... @michael-e and @jonmifsud suggested

Saving failed. The entry has been changed by Test on…

[ ] Don’t care about Joe’s changes? Yes I would much prefer if Joe’s changes never existed!

nitriques · 2017-05-19T01:56:29Z

@michael-e @jonmifsud @brendo would you mind checking the last commit (fc9adc4). @michael-e I think we need to tell the user at least why Saving "failed" (I prefer aborted since that what's happening)

michael-e · 2017-05-19T08:29:50Z

symphony/lib/toolkit/class.administrationpage.php

@@ -1261,7 +1262,9 @@ public function addTimestampError($errorMessage, $existingObject, $options = arr
        ));
        $errdiv->appendChild(new XMLElement('label',
            Widget::Input('action[ignore-timestamp]', null, 'checkbox')->generate() . ' ' .
-            __('Override? Please understand that data loss can occur.')
+            __("I don't care about %s's changes, pretend it never existed.", array(


Shouldn't this be "pretend they…" or "pretend these…"?

Anyway it sounds complicated. What about "I don't care about %s's changes. Save my version."

This copy sounds quite rude …

michael-e · 2017-05-19T08:32:35Z

symphony/content/content.blueprintssections.php

                return false;
            } else if (isset($_POST['action']['timestamp'])) {
                $tv = new TimestampValidator('sections');
                if (!$tv->check($section_id, $_POST['action']['timestamp'])) {
-                    $this->_errors['timestamp'] = __('The action requested was made on a earlier version of the record.');
+                    $this->_errors['timestamp'] = __('Saving aborted, data integrity check failed.');


"data integrity check failed" isn't very user-friendly, is it? Can we make this more clear?

brendo · 2017-05-19T12:19:29Z

Mm, I wonder if the in field approach could be refined. At the moment, any 'errors' that occur at a generic level are introduced with a page alert. We have a convention where links can appear as well, and clicking on the link perform actions (usually navigation).

Would it be suitable to instead display a page alert along the lines of:

This entry has been changed by %s %s. _Replace anyway?_ or _View %s's entry_

eg. This entry has been changed by Brendan a few moments ago. Replace anyway? or View Brendan's entry.

I'm not sure if the Brendan's bit translates well though, how does it read in German/French?

This cleans up the interface and doesn't introduce a new field, a pattern that doesn't exist. Items in the main area are always the result of field or extension, never by the core.

nilshoerrmann · 2017-05-19T12:25:25Z

I like this approach.

I'm not sure if the Brendan's bit translates well though, how does it read in German/French?

It's possible translation-wise: the German equivalent would be "Zeige Brendans Eintrag". But you'll run into trouble with names like mine which should be inserted as "View Nils' entry" and not "View Nils's entry". But it's possible to work that out with good copywriting.

brendo · 2017-05-19T12:33:00Z

But it's possible to work that out with good copywriting.

Yeah I'm wondering if other terms could work:

View changed entry
View changes (might imply diff like view though?)
View new entry
View updated entry
Show entry

nilshoerrmann · 2017-05-19T15:09:48Z

View other entry?

nitriques · 2017-05-19T20:15:33Z

Yes, debate, ideas! I like Brendon's idea. It was my first idea, but it felt wrong with a check box in it...
Let me send some more commits ;)

brendo · 2017-05-19T20:40:25Z

Why does it need a checkbox? Could it be hidden and it's value changed by JS? Or checked by CSS target

nitriques · 2017-05-24T14:10:46Z

Why does it need a checkbox? Could it be hidden and it's value changed by JS? Or checked by CSS target

It does not!!! Your solution is completely valid! I just did not thought of it!

(somehow, I only saw the checkbox option)

nitriques · 2017-05-26T21:39:30Z

@brendo There is even a bonus side effect of using the alert: We can display it in index pages as well!!!

nitriques · 2017-05-26T22:07:36Z

How about this :

nilshoerrmann · 2017-05-27T11:53:35Z

What about:

The entry could not be saved due to conflicting changes by X at Y. Dismiss changes? View changes.

brendo · 2017-05-28T01:02:03Z

I like @nilshoerrmann wording, it's succinct. My only worry is that "Dismiss changes" is too soft and doesn't clearly explain that you will essentially delete data. What about "Overwrite changes?" or "Replace changes"?

nilshoerrmann · 2017-05-28T05:33:18Z

Both suggestions sound just fine.

I had the German word “verwerfen” in mind which translates to “dismiss” or “discard“. So maybe you should just decide, Brendan, because as a native speaker you know best what works in the English language.

My main intention was to shorten Nicolas’ message in order to align the wording of both actions with the rest of the interface.

brendo · 2017-05-29T08:57:02Z

If it's up to me, I'd vote for Replace changes

nitriques · 2017-05-29T17:48:56Z

Yes I like this too!

Is it good to merge ?

nitriques · 2017-05-29T18:10:47Z

Is it good to merge ?

I meant can I work on finishing this feature since the UI is ok ? :P

nitriques · 2017-06-07T22:07:04Z

This thing will be an ongoing effort, since it's "complete" (meaning things that are implemented are working), I'll merge soon.

@brendo

We add a timestamp check before saving records, either in sections (fields) or entries. When saving a section, if the data changes between the render of the page and the form post, it can lead to data loss if a field was added. The SQL will crash because it assumes the fields did not change. When saving and entry, the SQL was ok, but still, it can lead to data loss. At both places, the fix provided here makes sure we are editing the lastest version of the record. Rebased from 81438f6~1...0c8c765 Rebased from 60e930e~1...1a931a4 Fix SQL error when saving a deleted field This commit makes sure that a request to update a field will not fail if the field as already been deleted by another user (or the same user in a different tab/browser). If the field is found to be non-existant, it is treated as a new field. Fixes symphonycms#2703 pick 1c84b21 Replace the added fieldset with an alert @brendo's suggestion is WAY better then the current implementation, which adds a new UI pattern in the system. Usign links in the alerts makes it possible to offer the overwrite mode, without gluttering the UI. In order to make it possible to submit the form from the alert, a little of pretty simple javascript was needed. Rebased from 6f37eaf~1...c652b18

pick be8bbb7

This is needed to properly display the name of the author that created the conflict. I first thought that the author_id would get updated on save, but it turns our it does not: it's the creation author. So in order to make it backward compatible a modification_author_id is needed. pick dff0d7f

This prevents SQL error in case no author can be found (import script by example). It re-use the default value already established on creation. Fixes symphonycms#2152 pick 64b2b7f

@brendo

We add a timestamp check before saving records, either in sections (fields) or entries. When saving a section, if the data changes between the render of the page and the form post, it can lead to data loss if a field was added. The SQL will crash because it assumes the fields did not change. When saving and entry, the SQL was ok, but still, it can lead to data loss. At both places, the fix provided here makes sure we are editing the lastest version of the record. Rebased from 81438f6~1...0c8c765 Rebased from 60e930e~1...1a931a4 Fix SQL error when saving a deleted field This commit makes sure that a request to update a field will not fail if the field as already been deleted by another user (or the same user in a different tab/browser). If the field is found to be non-existant, it is treated as a new field. Fixes #2703 pick 1c84b21 Replace the added fieldset with an alert @brendo's suggestion is WAY better then the current implementation, which adds a new UI pattern in the system. Usign links in the alerts makes it possible to offer the overwrite mode, without gluttering the UI. In order to make it possible to submit the form from the alert, a little of pretty simple javascript was needed. Rebased from 6f37eaf~1...c652b18 Picked from 95fb812

@brendo

We add a timestamp check before saving records, either in sections (fields) or entries. When saving a section, if the data changes between the render of the page and the form post, it can lead to data loss if a field was added. The SQL will crash because it assumes the fields did not change. When saving and entry, the SQL was ok, but still, it can lead to data loss. At both places, the fix provided here makes sure we are editing the lastest version of the record. Rebased from 81438f6~1...0c8c765 Rebased from 60e930e~1...1a931a4 Fix SQL error when saving a deleted field This commit makes sure that a request to update a field will not fail if the field as already been deleted by another user (or the same user in a different tab/browser). If the field is found to be non-existant, it is treated as a new field. Fixes #2703 pick 1c84b21 Replace the added fieldset with an alert @brendo's suggestion is WAY better then the current implementation, which adds a new UI pattern in the system. Usign links in the alerts makes it possible to offer the overwrite mode, without gluttering the UI. In order to make it possible to submit the form from the alert, a little of pretty simple javascript was needed. Rebased from 6f37eaf~1...c652b18 Picked from 95fb812

next() always returns an entry object or null, never an array. This problem was introduced in 154c161 Re #2659

next() always returns an entry object or null, never an array. This problem was introduced in 154c161 Re symphonycms#2659 Picked from 119f3fe

next() always returns an entry object or null, never an array. This problem was introduced in 154c161 Re #2659 Picked from 119f3fe Picked from d9992e9

next() always returns an entry object or null, never an array. This problem was introduced in 154c161 Re symphonycms#2659 Picked from 119f3fe Picked from d9992e9

nitriques added 1 - Minor Enhancement Needs Testing labels Apr 21, 2017

nitriques added this to the 2.7.0 milestone Apr 21, 2017

brendo reviewed Apr 22, 2017

View reviewed changes

nitriques force-pushed the 2.7.x-data-loss branch from bb88937 to 68603b8 Compare April 27, 2017 18:39

nitriques added the WIP (do not merge!) label May 17, 2017

michael-e reviewed May 19, 2017

View reviewed changes

nitriques mentioned this pull request May 29, 2017

Rebase 2.7.x on 2.6.x #2682

Closed

nitriques force-pushed the 2.7.x-data-loss branch 2 times, most recently from 845c71a to 82d4b3f Compare May 30, 2017 20:20

This was referenced Jun 1, 2017

Discarding SQL errors when editing entries #2401

Closed

Field integrity tests #2703

Closed

nitriques added Feedback needed and removed WIP (do not merge!) labels Jun 7, 2017

nitriques added 4 commits June 14, 2017 13:41

Make sure author_id is an int

f29dd28

pick be8bbb7

Add a default value of author_id

af75b3c

This prevents SQL error in case no author can be found (import script by example). It re-use the default value already established on creation. Fixes symphonycms#2152 pick 64b2b7f

nitriques force-pushed the 2.7.x-data-loss branch from 64b2b7f to af75b3c Compare June 14, 2017 20:42

nitriques removed the Feedback needed label Jun 14, 2017

nitriques merged commit af75b3c into symphonycms:2.7.x Jun 14, 2017

nitriques deleted the 2.7.x-data-loss branch June 14, 2017 20:43

nitriques added a commit that referenced this pull request Oct 9, 2018

Fix error introduced by code rewrite

119f3fe

next() always returns an entry object or null, never an array. This problem was introduced in 154c161 Re #2659

nitriques added a commit to DeuxHuitHuit/symphonycms that referenced this pull request Mar 29, 2019

Fix error introduced by code rewrite

d9992e9

next() always returns an entry object or null, never an array. This problem was introduced in 154c161 Re symphonycms#2659 Picked from 119f3fe

nitriques added a commit that referenced this pull request Apr 8, 2019

Fix error introduced by code rewrite

0065b38

next() always returns an entry object or null, never an array. This problem was introduced in 154c161 Re #2659 Picked from 119f3fe Picked from d9992e9

Prevent data loss on save (entries and sections) #2659

Prevent data loss on save (entries and sections) #2659

Conversation

nitriques commented Apr 21, 2017

brendo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitriques Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmifsud commented Apr 23, 2017

nitriques commented Apr 25, 2017

michael-e commented Apr 26, 2017

nitriques commented Apr 26, 2017

michael-e commented Apr 27, 2017

jonmifsud commented Apr 27, 2017 via email

nitriques commented Apr 27, 2017

nitriques commented Apr 27, 2017

michael-e commented Apr 27, 2017

michael-e commented Apr 27, 2017

nitriques commented Apr 27, 2017

nitriques commented Apr 27, 2017

nitriques commented Apr 27, 2017 • edited Loading

nitriques commented May 19, 2017

Choose a reason for hiding this comment

nilshoerrmann May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendo commented May 19, 2017

nilshoerrmann commented May 19, 2017

brendo commented May 19, 2017

nilshoerrmann commented May 19, 2017

nitriques commented May 19, 2017

brendo commented May 19, 2017

nitriques commented May 24, 2017 • edited Loading

nitriques commented May 26, 2017

nitriques commented May 26, 2017

nilshoerrmann commented May 27, 2017 • edited Loading

brendo commented May 28, 2017

nilshoerrmann commented May 28, 2017

brendo commented May 29, 2017

nitriques commented May 29, 2017

nitriques commented May 29, 2017

nitriques commented Jun 7, 2017

nitriques Apr 26, 2017 •

edited

Loading

nitriques commented Apr 27, 2017 •

edited

Loading

nilshoerrmann May 19, 2017 •

edited

Loading

nitriques commented May 24, 2017 •

edited

Loading

nilshoerrmann commented May 27, 2017 •

edited

Loading