1086519 - Warn the user before letting them save a cloned page as "Original_title_clone"

Status: RESOLVED FIXED

(developer.mozilla.org Graveyard :: Wiki pages, enhancement)

Description

[Eric Shepherd [:sheppy]]

What feature should be changed? Please provide the URL of the feature if possible.
==================================================================================
When cloning a page, the default slug shouldn't be the original slug with "_clone" appended to the end. OR we should use an alert to remind the user that they didn't change it, offering them a chance to fix it before doing the clone.

What problems would this solve?
===============================
We occasionally get pages like this created, which eventually results in confusion, additions to the docs done in the wrong place, and automatically generated menus that list things more than once because of the clones.

Who would use this?
===================
Most contributors to MDN, eventually.

What would users see?
=====================
Either the slug field should start out empty, or an alert should be used to warn the user about this problem when appropriate.

What would users do? What would happen as a result?
===================================================
Attempting to clone a page should work smoothly but make some attempt to prevent the user from accidentally saving it with "clone" in the slug.

Is there anything else we should know?
======================================
I've just deleted https://developer.mozilla.org/en-US/Persona_clone because of this problem. That page having that slug and the title "Persona" resulted in the main Mozilla docs menu listing Persona twice; once from https://developer.mozilla.org/en-US/Persona and once from https://developer.mozilla.org/en-US/Persona_clone.

Comment 1

[David Walsh :davidwalsh]

Easy change and we should start the slug empty, IMO

Comment 2

[Eric Shepherd [:sheppy]]

(In reply to David Walsh :davidwalsh from comment #1)
> Easy change and we should start the slug empty, IMO

I agree.

Comment 3

[Luke Crouch [:groovecoder]]

Assigning mdholloway who has offered to do this.

Comment 4

[Michael Holloway [:mdholloway]]

Looking at the new_document(request) function in /kuma/kuma/wiki/views.py, is there ever a case in which a new page will have a non-empty value for initial_slug except when it's cloned?  If there are and we want to preserve the variable for other situations, it looks like just deleting this line:

initial_slug = _split_slug(clone_doc.slug)['specific'] + '_clone' 

in the if clone_id block (lines 891-904) will take care of this issue.  Otherwise it looks like there are a handful of other references to initial_slug to take out.

Also, the initial title for a cloned page depends on the initial slug, so doing the single-line fix would mean that the initial title for the cloned page would be blank as well.  Is this what we want?  That seems sensible to me based on the issues raised in the initial bug report, but there's currently a test to make sure cloned pages come in with a title.

Comment 5

[Luke Crouch [:groovecoder]]

Thanks for the spelunking, Michael!

There's another case where we provide an initial_slug: when we redirect from 404 pages to the docs/new?slug= for signed-in users to create the page:

https://github.com/mozilla/kuma/blob/master/kuma/wiki/views.py#L221-240
https://github.com/mozilla/kuma/blob/master/kuma/wiki/views.py#L852

But, we should still be able to clear it down in the clone page logic, so I think it's fine.

:sheppy - is it okay if a cloned page's title starts empty too?

Comment 6

[Eric Shepherd [:sheppy]]

(In reply to Luke Crouch [:groovecoder] from comment #5)
> Thanks for the spelunking, Michael!
> 
> There's another case where we provide an initial_slug: when we redirect from
> 404 pages to the docs/new?slug= for signed-in users to create the page:
> 
> https://github.com/mozilla/kuma/blob/master/kuma/wiki/views.py#L221-240
> https://github.com/mozilla/kuma/blob/master/kuma/wiki/views.py#L852
> 
> But, we should still be able to clear it down in the clone page logic, so I
> think it's fine.
> 
> :sheppy - is it okay if a cloned page's title starts empty too?

Yes, in fact; that's probably actually the best behavior.

Comment 7

[Luke Crouch [:groovecoder]]

Thanks :sheppy. Michael - is that enough to un-block you?

Comment 8

[Michael Holloway [:mdholloway]]

Yes, thanks for the info - I'll submit a patch shortly.

Comment 9

[Michael Holloway [:mdholloway]]

Attachment: 0002-remove-line-setting-initial_slug-for-cloned-pages.patch

Seems like this should do it.  But I should mention that this makes one of the automated tests fail:

======================================================================
FAIL: test_clone (kuma.wiki.tests.test_views.DocumentEditingTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vagrant/src/kuma/wiki/tests/test_views.py", line 1794, in test_clone
    eq_(page.find('input[name=title]')[0].value, title)
AssertionError: None != 'My Doc'

----------------------------------------------------------------------

Do I need to worry about this?  Looks like the failing behavior is the one we decided we wanted.

Comment 10

[Luke Crouch [:groovecoder]]

Michael, yeah that looks like a legitimate test failure caused by the change.

By the way, if you can send a Pull Request via GitHub with the change, Travis CI will automatically run tests, docs, and style checks on it and generate reports that everyone can see.

Are you able to dig into the test failure to debug it? I.e., to decide if the code change is right and the test needs to be updated, or if the test is right?

Comment 11

[Luke Crouch [:groovecoder]]

Comment on attachment 8562218 [details] [diff] [review]
0002-remove-line-setting-initial_slug-for-cloned-pages.patch

Review of attachment 8562218 [details] [diff] [review]:
-----------------------------------------------------------------

The test failure looks legitimate.

Comment 12

[Michael Holloway [:mdholloway]]

I'm having some Vagrant issues for some reason -- persisting even after completely uninstalling and reinstalling Vagrant -- so haven't been able to do any more manual testing, but here's the relevant test in test_views.py:

def test_clone(self):
        self.client.login(username='admin', password='testpass')
        slug = 'my_doc'
        title = 'My Doc'
        content = '<p>Hello!</p>'

        test_revision = revision(save=True, title=title, slug=slug,
                                 content=content)
        document = test_revision.document

        response = self.client.get(reverse('wiki.new_document',
                                           args=[],
                                           locale=settings.WIKI_DEFAULT_LANGUAGE) + '?clone=' + str(document.id))

        page = pq(response.content)

        eq_(page.find('input[name=title]')[0].value, title)
        eq_(page.find('input[name=slug]')[0].value, slug + '_clone')
        eq_(page.find('textarea[name=content]')[0].value, content)

I'm guessing at some of the syntax here, but it looks like it's just testing to see whether the name of a title, slug and value of a cloned page are the same as in the original.  Does this seem right to you?

There's one other test that fails when I run the suite, but this one (after looking at the underlying tests) I'm more confident has nothing to do with my change:

======================================================================
FAIL: test_dont_send_untranslated_language_email (kuma.users.tests.test_tasks.TestWelcomeEmails)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vagrant/src/vendor/packages/mock/mock.py", line 196, in patched
    return func(*args, **keywargs)
  File "/home/vagrant/src/kuma/users/tests/test_tasks.py", line 38, in test_dont_send_untranslated_language_email
    eq_(1, len(mail.outbox))
AssertionError: 1 != 0

Comment 13

[Luke Crouch [:groovecoder]]

Michael, thanks for the update. Sorry your vagrant setup is having trouble. :(

Are you comfortable sending this as a pull request on GitHub? That will make it easy for me to check out your branch, run tests, and help you debug.

Comment 14

[Michael Holloway [:mdholloway]]

Hey, sorry about the delay.  I created a pull request just now; see https://github.com/mozilla/kuma/pull/3096. 

On Vagrant: I was able to get things running earlier by updating my local code from mozilla/kuma.git, but now I'm getting errors again even after such an update.  Hmm...

Comment 15

[Michael Holloway [:mdholloway]]

Eric, I just want to confirm that the title should start empty as well as the slug -- currently leaving the title empty causes a test failure (see https://github.com/mozilla/kuma/pull/3096#issuecomment-77194006).  If so, let me know and I'll change test_views.py accordingly.

Comment 16

[Luke Crouch [:groovecoder]]

:sheppy - should we start the title empty as well?

Comment 17

[Luke Crouch [:groovecoder]]

:sheppy - what's the call on this? We've got a week-old pull request on it now waiting for this answer.

Comment 18

[Eric Shepherd [:sheppy]]

Sorry everyone, my bugmail got caught in limbo during my ongoing email crisis and I just got to this one. Yes, that's the behavior we want in this case. And of course, there should be an error if you try to save with an empty title, but I expect that check exists already.

Comment 19

[MDN Team (:mdn-dev)]

Commits pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/f5d142e03781c5e57d4b029166cef28795adb22d
Bug 1086519 - Leave new title and slug for cloned pages blank

https://github.com/mozilla/kuma/commit/da1d7c973f572289888473e6c11d9102c877a057
Bug 1086519 - Edit test_clone to expect empty title and slug

https://github.com/mozilla/kuma/commit/6b403dafacfbf57610dedfa0e5f2c13b4d8fe2ce
Merge pull request #3128 from mdholloway/master

Bug 1086519 - Leave new title and slug for cloned pages blank [take 2]

Comment 20

[Michael Holloway [:mdholloway]]

Hey, should I close this bug?

Comment 21

[Luke Crouch [:groovecoder]]

Oh, yes we can close it. Thanks Michael!

Comment 22

[Luke Crouch [:groovecoder]]

Comment on attachment 8562218 [details] [diff] [review]
0002-remove-line-setting-initial_slug-for-cloned-pages.patch

Review of attachment 8562218 [details] [diff] [review]:
-----------------------------------------------------------------

Reviewed via GitHub.