Add revision tracking to drafts, remove button, remove from new page #4186

stephaniehobson · 2017-04-20T01:32:05Z

The goal of this PR is to make the auto save drafting feature reliable.

NOT READY. But this is a big PR and I would appreciate a first pass review while I fight the edge cases.

Still to do:

Fix #todo links for revision history
Write tests

Fix Bug 1264314: Alert user if page content has changed since last draft

revision ID is now saved when draft saved
displays warning message if revision ID is old
displays note if revision ID is old
- Fix Bug 1298399: Remove ?document_saved
query variable now includes revision ID of draft to delete
query variable now directly consumed by javascript
query variable new removed after draft deleted
- Fix Bug 1268906: Tool bar buttons now trigger draft saves
changed listener from 'key' to 'change'
- Fix Bug 1274067: No new drafts created after discarding page
disabled autosave after discard button clicked
- Fix Bug 1269851: Save draft should not wait for typing break to save
added throttled call to autosave every 500ms
reduced debounce autosave to 500ms, to catch edits made after throttle
- Fix Bug 1301656: Remove draft feature from new page creation
removed draft widget from new pages
changed data format for new translation pages to retain drafts there

Also:

lint wiki-edit.js
remove unused wiki-section-edit.js
autosave now enabled by default even with saved draft
- removed need to discard saved draft
separated draft functionality into a new file
new utility function to get query strings by variable name
added some analytics instrumentation
standardized draft storage formats for non-English pages
- migrated old draft formats to new storage format

stephaniehobson · 2017-04-20T01:36:11Z

@schalkneethling I'd appreciate an initial review. I'm still fighting with some edge cases and need to add tests.

@jwhitlock You know the weird rabbit warrens that are the create / edit / translate paths in kuma, can you look and see if I've left an edge case unaccounted for? Create English pages should not get drafts anymore, new translations should, edits should, and translation edits should.

If I work on this at all tomorrow it will be tests, so no hurry on the review. I have a few reviews to do myself ;)

schalkneethling · 2017-04-21T12:42:20Z

kuma/static/js/wiki.js

+    // Generates a storage key based on pathname
+    // copied from wiki-edit-draft.js because: race conditions
+    // does not need to copy logic for dealing with new translations
+    //


We should probably start moving to some cross project standards for comments. In this case we probably want to do:

/** * Generates a storage key based on pathname * copied from wiki-edit-draft.js because: race conditions * does not need to copy logic for dealing with new translations */

Probably a good idea to follow the JSDoc guidelines http://usejsdoc.org/ - unless MDN already uses another JSDoc standard or tool.

MDN is the wild west of javascript formatting right now (I just copy what is in the file I'm working on at the time). Cross project standards would be awesome. 👍

I will try to get over my distaste for using comments that are not left aligned :P

stephaniehobson · 2017-04-22T00:15:32Z

Darn, legitimate test failure. That's what I get for editing Python...

jwhitlock · 2017-04-25T19:28:18Z

kuma/static/js/wiki-edit-draft.js

+    // some knowledge about the page we're dealing with
+    var $translateForm = $('#translate-document');
+    var $form = $translateForm.length ? $translateForm : $('#wiki-page-edit');
+    var isNewTranslation = location.search.indexOf('?tolocale') > -1 ? true : false;


Will tolocale always be the first query string parameter?

Not sure, but this should be okay if it's not.

jwhitlock · 2017-04-25T19:28:49Z

kuma/static/js/main.js

+        var regex = new RegExp('[\\?&]' + name + '=([^&#]*)');
+        var results = regex.exec(location.search);
+        return results === null ? '' : decodeURIComponent(results[1].replace(/\+/g, ' '));
+    };


JavaScript doesn't have a native method for this. 😢

jwhitlock · 2017-04-25T19:30:31Z

kuma/static/js/wiki-edit-draft.js

+    var draftAutoSaveEnabled = true;
+    var draftWait = false;
+    var draftWaitTime = 500;
+    var $revisionInputs = $('[name=current_rev]'); // can't use ID because there's a duplicate >_<


We could try to fix this in the form / template logic...

That would be nice. It broke things when I tried and it's an ancient problem so I figured it could be dealt with separately but if you want to pair on this we could try.

jwhitlock · 2017-04-25T19:31:36Z

kuma/static/js/wiki-edit-draft.js

+            // change translation creation format to match edit format
+            // /en-US/docs/page$translate?tolocale=fr
+            // draft/edit/fr/docs/page
+            var localeCode = location.search.replace('?tolocale=', '');


could use the new getUrlParameter

jwhitlock · 2017-04-25T19:35:43Z

kuma/static/js/wiki-edit-draft.js

+                draftWarning += '<div class="readable-line-length">';
+                // long line is long, but breaking it up wrecks it for the localizers
+                draftWarning += gettext('Compare this date to the latest revision date to ensure you\'re not overwriting later changes.');
+                draftWarning += ' <a href="#TODO" class="external-icon">Revision history.</a>';


Revision history should be localized. Is the #TODO for this PR?

jwhitlock · 2017-04-25T19:36:57Z

kuma/static/js/wiki-edit-draft.js

+                draftWarning += '<div class="readable-line-length">';
+                // long line is long, but breaking it up wrecks it for the localizers
+                draftWarning += gettext('A newer version of this article has been published since this draft was saved. You can restore the draft to view the content, but you will not be able to submit it for publishing.');
+                draftWarning += ' <a href="#TODO" class="external-icon">Revision history.</a> ';


Revision history should be localized. Is the #TODO for this PR?

jwhitlock · 2017-04-25T19:37:15Z

kuma/static/js/wiki-edit-draft.js

+                // long line is long, but breaking it up wrecks it for the localizers
+                draftWarning += gettext('A newer version of this article has been published since this draft was saved. You can restore the draft to view the content, but you will not be able to submit it for publishing.');
+                draftWarning += ' <a href="#TODO" class="external-icon">Revision history.</a> ';
+                draftWarning += '<a href="#TODO" class="external-icon">Published version</a>';


Published version should be localized. Is the #TODO for this PR?

jwhitlock · 2017-04-25T19:44:47Z

kuma/static/js/wiki-edit-draft.js

+            $draftStatus.append(gettext('Draft') + ' ')
+                .append($updateAction)
+                .append(' ')
+                .append($updateTime);


I'm not sure localized strings can be combined this way in all locales. It may be better to pass the two values in ('published' and 'discarded') and construct translatable strings for each, like

if (action === 'published') { $draftStatus.append(interpolate("Draft published at %s", $updateTime)) }

I'm 100% sure they can't be but this is what was there before.

jwhitlock · 2017-04-25T19:45:44Z

kuma/static/js/wiki-edit-draft.js

+        // disable autosave when saving manually
+        enableAutoSave(false);
+    }
+    */


Remove, if you've gotten consensus to remove manual draft saving.

jwhitlock · 2017-04-25T19:46:22Z

Ug, thought I was leaving a review. Sorry for the spam...

jwhitlock

This worked well in practical testing. It was awesome to see all the old local storage keys switch to the new format. It would be nice to throw away the old ones as well, but good work preserving them.

I tried:

New page - no drafts saved
Edit page - Drafts auto-saved, recovered, discarded. The revision in the final URLs was removed by JS, as well as the draft.
Translate page - Drafts auto-saved. There was an empty revision number, and the draft was deleted on save.
Translation Edit - Drafts auto-saved. Revision number was included in the URL after saving, and was removed by JS, as well as the draft.

I added a lot of comments to the JS code, which I think can mostly be ignored, since this is moving code around and fixing the formatting. All of page editing needs to be re-written, back and front end. But not in this PR.

jwhitlock · 2017-04-25T20:41:52Z

kuma/static/js/wiki-edit.js

            // disable form
            $('#wiki-page-edit').attr('disabled', true);

            // Clear previous notification messages, if any
            $('.notification button.close').click();

            // give user feedback
-            var saveNotification = mdn.Notifier.growl('Saving changes…', { duration: 0 , closable: true});
+            var saveNotification = mdn.Notifier.growl('Publishing changes…', { duration: 0 , closable: true});


wrap in gettext?

jwhitlock · 2017-04-25T20:44:04Z

kuma/static/js/wiki.js

+    /**
+     * Generates a storage key based on pathname
+     * copied from wiki-edit-draft.js because: race conditions
+     * does not need to copy logic for dealing with new translations


👍 explaining duplication

jwhitlock · 2017-04-25T20:50:13Z

kuma/wiki/views/edit.py

@@ -268,7 +268,7 @@ def edit(request, document_slug, document_locale, revision_id=None):
                            # content API, constrain to that section.
                            params['section'] = section_id
                    # Parameter for the document saved, so that we can delete the cached draft on load
-                    params['document_saved'] = 'true'
+                    params['rev_saved'] = curr_rev.id


If this is a document without any revisions, then cur_rev will be null, and this will raise an exception. I'm not sure how this would be possible, outside of tests. However, there is one in production (https://developer.mozilla.org/de/docs/Glossary/Raster), and several in the sample database (http://localhost:8000/en-US/docs/Project:MDN/Contributing).

I think if curr_rev is None, you can do whatever you want. This edit view is in need of a re-write, as well as the two tests exercising this invalid code path.

jwhitlock · 2017-04-25T21:19:05Z

kuma/wiki/views/translate.py

+                url = doc.get_absolute_url()
+                params = {}
+                # Parameter for the document saved, so that we can delete the cached draft on load
+                params['rev_saved'] = request.POST.get('current_rev', '')


For the first translation, current_rev will be empty, and the query string will be rev_saved=. I think this is what you want, but you'll need to update the translation tests to expect it.

Tests updated 👍

stephaniehobson · 2017-05-03T00:05:26Z

Should be down to one test failure.

stephaniehobson · 2017-05-03T23:47:25Z

Okay, ready for final reviews now!

Still writing tests...

jwhitlock · 2017-05-04T12:32:43Z

kuma/wiki/tests/test_views.py

-                                                       args=[edit_doc.slug]))
+                doc_url = reverse('wiki.document', args=[edit_doc.slug], locale=foreign_locale)
+                params = {'rev_saved': ''}
+                doc_url = '%s?%s' % (doc_url, urlencode(xparams))


This should be params not xparams, as caught by flake8. It's also code that isn't called during tests, but we agreed to postpone cleanup until after this PR is merged.

codecov-io · 2017-05-04T23:14:13Z

Codecov Report

Merging #4186 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4186      +/-   ##
==========================================
+ Coverage   87.08%   87.09%   +<.01%     
==========================================
  Files         155      155              
  Lines        9415     9420       +5     
  Branches     1263     1263              
==========================================
+ Hits         8199     8204       +5     
  Misses        989      989              
  Partials      227      227

Impacted Files	Coverage Δ
kuma/settings/common.py	`94.28% <ø> (ø)`	⬆️
kuma/wiki/views/edit.py	`69.93% <100%> (ø)`	⬆️
kuma/wiki/views/translate.py	`94.36% <100%> (+0.2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce08787...b5e34e7. Read the comment docs.

stephaniehobson · 2017-05-08T21:16:09Z

@schalkneethling Did you want to take another look at this before it goes live? The code is "final" now I'm just writing tests.

schalkneethling · 2017-05-09T07:02:17Z

Looks good to me @stephaniehobson r+

stephaniehobson · 2017-05-26T22:38:17Z

I think this is good to go. @jwhitlock I think I have addressed the changes you requested.

jwhitlock · 2017-05-26T23:04:54Z

😄 will test it out Tuesday morning

stephaniehobson · 2017-05-27T00:13:44Z

Tests still failing :( I thought we had that beat. So much for my triumphant weekend.

jwhitlock · 2017-05-30T14:10:15Z

kuma/wiki/views/edit.py

-                    # if we're not getting raw content.
-                    url = '%s#%s' % (url, section_id)
-                return redirect(url)
+                    orig_rev = Revision.objects.get(pk=orig_rev_id)


This rebase pulled in too much code. Lines 248-307 should be removed, and then the redirect URL code re-indented.

jwhitlock

Looking good. There is a functional test that is broken, a few except blocks that can be more focused, and a few nits.

I also need to make these tests run better against local dev, but future goals.

jwhitlock · 2017-05-30T18:02:56Z

kuma/wiki/views/translate.py

+                url = doc.get_absolute_url()
+                params = {}
+                # Parameter for the document saved, so that we can delete the cached draft on load
+                params['rev_saved'] = request.POST.get('current_rev', '')


Tests updated 👍

jwhitlock · 2017-05-30T19:16:39Z

tests/functional/test_article_new.py

+    # wait for throttled draft function to activate (hopefully not)
+    time.sleep(0.6)
+    # check save draft not activated
+    assert not page.is_draft_container_displayed


This should be page.editor().is_draft_container_displayed.

jwhitlock · 2017-05-30T19:19:03Z

tests/pages/regions/ckeditor.py

+    def is_draft_old_displayed(self):
+        return self.find_element(
+            *self._draft_old_locator
+        ).is_displayed()


This will raise an exception if the element doesn't exist, which is being tested in test_article_new. I'd add an import at the top:

from selenium.common.exceptions import NoSuchElementException

and then write it like:

def is_draft_old_displayed(self): try: return self.find_element( *self._draft_old_locator ).is_displayed() except NoSuchElementException: return False

jwhitlock · 2017-05-30T19:19:38Z

tests/pages/regions/ckeditor.py

+            return self.find_element(
+                *self._draft_restore_locator
+            ).is_displayed()
+        except:


except: is too general, please make this except NoSuchElementException:

jwhitlock · 2017-05-30T19:19:58Z

tests/pages/regions/ckeditor.py

+    def is_draft_status_displayed(self):
+        try:
+            return self.find_element(*self._draft_status_locator).is_displayed()
+        except:


except NoSuchElementException: here as well

jwhitlock · 2017-05-30T19:26:18Z

tests/pages/article_new.py

-            **self.url_kwargs
+            locale=self.DEFAULT_LOCALE,
+            slug=self.DOC_SLUG,
+            # **self.url_kwargs


This comment can be deleted

jwhitlock · 2017-05-30T19:28:13Z

tests/pages/article.py

        edit_button = self.find_element(*self._edit_button_locator)
        edit_button.click()
        if (signedin):
-            from pages.article_edit import EditPage


👍 removing this import

jwhitlock · 2017-05-30T19:29:49Z

tests/pages/regions/ckeditor.py


-    def edit_source(self):
+    def edit_source(self, content=TEST_TEXT):


👍 making this an optional parameter instead of a class constant

The goal of this PR is to make the auto save drafting feature reliable. Fix Bug 1264314: Alert user if page content has changed since last draft - revision ID is now saved when draft saved - displays warning message if revision ID is old - displays note if revision ID is old Fix Bug 1298399: Remove ?document_saved - query variable now includes revision ID of draft to delete - query variable now directly consumed by javascript - query variable new removed after draft deleted Fix Bug 1268906: Tool bar buttons now trigger draft saves - changed listener from 'key' to 'change' Fix Bug 1274067: No new drafts created after discarding page - disabled autosave after discard button clicked Fix Bug 1269851: Save draft should not wait for typing break to save - added throttled call to autosave every 500ms - reduced debounce autosave to 500ms, to catch edits made after throttle Fix Bug 1301656: Remove draft feature from new page creation - removed draft widget from new pages - changed data format for new translation pages to retain drafts there Also: - autosave now enabled by default even with saved draft -- removed need to discard saved draft - separated draft functionality into a new file - new utility function to get query strings by variable name - added some analytics instrumentation - standardized draft storage formats for non-English pages -- migrated old draft formats to new storage format

stephaniehobson · 2017-05-31T21:00:50Z

Made requested changes to tests 🎉

jwhitlock

🚢 SHIP IT

stephaniehobson added the not ready label Apr 20, 2017

stephaniehobson requested a review from schalkneethling April 20, 2017 01:33

stephaniehobson requested a review from jwhitlock April 20, 2017 01:36

schalkneethling reviewed Apr 21, 2017

View reviewed changes

stephaniehobson force-pushed the document-saved branch from 383f0e7 to cfbf24d Compare April 21, 2017 22:43

jwhitlock reviewed Apr 25, 2017

View reviewed changes

jwhitlock suggested changes Apr 25, 2017

View reviewed changes

stephaniehobson force-pushed the document-saved branch from cfbf24d to 2b7844f Compare May 3, 2017 00:04

stephaniehobson force-pushed the document-saved branch 3 times, most recently from 38a44fe to 17d3715 Compare May 3, 2017 23:46

stephaniehobson added needs tests and removed not ready labels May 3, 2017

jwhitlock reviewed May 4, 2017

View reviewed changes

stephaniehobson force-pushed the document-saved branch from 17d3715 to eb20688 Compare May 4, 2017 23:01

jwhitlock assigned stephaniehobson May 15, 2017

stephaniehobson added 2 commits May 24, 2017 14:41

Remove unused wiki-section-edit.js

66d91b5

Lint wiki-edit.js

1f80008

stephaniehobson force-pushed the document-saved branch from eb20688 to b1ac21d Compare May 26, 2017 22:32

stephaniehobson removed the needs tests label May 26, 2017

stephaniehobson changed the title ~~[DO NOT MERGE] Add revision tracking to drafts, remove button, remove from new page~~ Add revision tracking to drafts, remove button, remove from new page May 26, 2017

jwhitlock reviewed May 30, 2017

View reviewed changes

stephaniehobson force-pushed the document-saved branch 2 times, most recently from e19a6aa to 177c1b1 Compare May 30, 2017 15:49

jwhitlock suggested changes May 30, 2017

View reviewed changes

stephaniehobson force-pushed the document-saved branch from 177c1b1 to b5e34e7 Compare May 31, 2017 21:00

jwhitlock approved these changes May 31, 2017

View reviewed changes

jwhitlock merged commit 754f61b into master May 31, 2017

stephaniehobson deleted the document-saved branch June 7, 2017 23:58

mdn deleted a comment from beanoagin Jun 13, 2017


		def edit_source(self):
		def edit_source(self, content=TEST_TEXT):

Add revision tracking to drafts, remove button, remove from new page #4186

Add revision tracking to drafts, remove button, remove from new page #4186

Conversation

stephaniehobson commented Apr 20, 2017 • edited

stephaniehobson commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephaniehobson commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock Apr 25, 2017 • edited

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

jwhitlock Apr 25, 2017 • edited

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

Choose a reason for hiding this comment

jwhitlock commented Apr 25, 2017

jwhitlock 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephaniehobson commented May 3, 2017

stephaniehobson commented May 3, 2017

Choose a reason for hiding this comment

codecov-io commented May 4, 2017 • edited

Codecov Report

stephaniehobson commented May 8, 2017

schalkneethling commented May 9, 2017

stephaniehobson commented May 26, 2017

jwhitlock commented May 26, 2017

stephaniehobson commented May 27, 2017

Choose a reason for hiding this comment

jwhitlock 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

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

stephaniehobson commented May 31, 2017

jwhitlock left a comment

Choose a reason for hiding this comment

stephaniehobson commented Apr 20, 2017 •

edited

jwhitlock Apr 25, 2017 •

edited

jwhitlock Apr 25, 2017 •

edited

codecov-io commented May 4, 2017 •

edited