Remove a Language #100

arifin4web · 2017-07-27T08:22:29Z

As suggested in the issue #10 pre_delete signal couldn't be used. The signal doesn't Trigger before raising ProtectedError. Therefore Implemented a custom delete view for LanguageModelAdmin which will:

Show a clear warning to the user, which related Translatable pages are going to be removed.
Remove those pages and language itself.

codecov · 2017-07-27T08:38:35Z

Codecov Report

Merging #100 into master will increase coverage by 0.36%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage    89.5%   89.86%   +0.36%     
==========================================
  Files          14       16       +2     
  Lines         505      523      +18     
==========================================
+ Hits          452      470      +18     
  Misses         53       53

Impacted Files	Coverage Δ
...gtailtrans/templatetags/languages_wagtail_admin.py	`100% <100%> (ø)`
src/wagtailtrans/views/language.py	`100% <100%> (ø)`

mikedingjan · 2017-07-27T14:20:29Z

src/wagtailtrans/templates/modeladmin/wagtailtrans/language/delete.html

@@ -0,0 +1,41 @@
+{% extends "wagtailadmin/base.html" %}


Please extend from the existing delete.html template. This way you only have to override content_main and we keep the most of wagtails interface intact.

mikedingjan · 2017-07-27T14:21:49Z

src/wagtailtrans/templates/modeladmin/wagtailtrans/language/delete.html

+                <p><a href="{{ view.index_url }}" class="button">{% trans 'Go back to listing' %}</a></p>
+            {% else %}
+                <p>{{ view.confirmation_message }}</p>
+                {% with related_pages=view.get_linked_pages %}


When removing a canonical_page we list the related pages with a templatetag, imo it's better to have the same approach here.. The codebase stays more consistent that way

Agreed. Also this way it'll be more reusable.

mikedingjan · 2017-07-27T14:24:23Z

src/wagtailtrans/views/language.py

+        translatable pages (if there is any) before we can delete a Language
+        which are references through protected foreign key.
+        """
+        try:


First check if there are related pages, remove them and finally call the super method which will delete the language object.

mikedingjan · 2017-07-27T14:25:37Z

src/wagtailtrans/views/language.py

+        try:
+            self.delete_instance()
+        except models.ProtectedError:
+            self.remove_related_pages()


This method is a one-liner, it's fine by me to just call the delete here

mikedingjan · 2017-07-28T07:28:46Z

src/wagtailtrans/templatetags/languages_wagtail_admin.py

@@ -0,0 +1,24 @@
+import django


Can we try not to create too much templatetag files, I think having 2 files one: wagtailtrans_tags to be used by project implementation the other: wagtailtrans_admin_tags to store all the tags needed in the wagtail admin?

mikedingjan · 2017-07-28T07:29:15Z

src/wagtailtrans/templatetags/languages_wagtail_admin.py

+
+
+@assignment_tag
+def get_language_relate_pages(language):


better name would be get_related_pages_for_language

arifin4web · 2017-08-01T09:02:37Z

reviews are implemented! @mikedingjan

arifin4web · 2017-08-09T16:48:59Z

Whats the update on this one? can this be merged now?

jjanssen · 2017-08-14T17:23:38Z

Hi @arifin4web,
It is awaiting review and I want to take it for a spin really soon.

jjanssen · 2017-08-22T10:57:25Z

I'm getting the following stacktrace when I click on delete language:

TemplateSyntaxError at /admin/wagtailtrans/language/delete/3/
Invalid block tag on line 15: 'get_language_relate_pages', expected 'endif'. Did you forget to register or load this tag?

Can we fix this and add a test for that?

Stacktrace

Environment:


Request Method: GET
Request URL: http://localhost:8000/admin/wagtailtrans/language/delete/3/

Django Version: 1.10.5
Python Version: 2.7.10
Installed Applications:
['django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'wagtail.contrib.settings',
 'wagtail.contrib.modeladmin',
 'wagtail.wagtailforms',
 'wagtail.wagtailredirects',
 'wagtail.wagtailembeds',
 'wagtail.wagtailsites',
 'wagtail.wagtailusers',
 'wagtail.wagtailsnippets',
 'wagtail.wagtaildocs',
 'wagtail.wagtailimages',
 'wagtail.wagtailsearch',
 'wagtail.wagtailadmin',
 'wagtail.wagtailcore',
 'wagtailtrans',
 'modelcluster',
 'taggit',
 'tests._sandbox.pages',
 'tests._sandbox.search']
Installed Middleware:
['django.middleware.security.SecurityMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.auth.middleware.SessionAuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'wagtail.wagtailcore.middleware.SiteMiddleware',
 'wagtail.wagtailredirects.middleware.RedirectMiddleware',
 'wagtailtrans.middleware.TranslationMiddleware']


Template error:
In template /Users/j.janssen/Sites/git-projects/wagtailtrans/src/wagtailtrans/templates/modeladmin/wagtailtrans/language/delete.html, error at line 15
   Invalid block tag on line 15: 'get_language_relate_pages', expected 'endif'. Did you forget to register or load this tag?   5 :     <div class="nice-padding">
   6 :         {% if protected_error %}
   7 :             <h2>{% blocktrans with view.verbose_name|capfirst as model_name %}{{ model_name }} could not be deleted{% endblocktrans %}</h2>
   8 :             <p>{% blocktrans with instance as instance_name %}'{{ instance_name }}' is currently referenced by other objects, and cannot be deleted without jeopardising data integrity. To delete it successfully, first remove references from the following objects, then try again:{% endblocktrans %}</p>
   9 :             <ul>
   10 :                 {% for obj in linked_objects %}<li><b>{{ obj|get_content_type_for_obj|title }}:</b> {{ obj }}</li>{% endfor %}
   11 :             </ul>
   12 :             <p><a href="{{ view.index_url }}" class="button">{% trans 'Go back to listing' %}</a></p>
   13 :         {% else %}
   14 :             <p>{{ view.confirmation_message }}</p>
   15 :              {% get_language_relate_pages instance as related_pages %} 
   16 :             {% if related_pages %}
   17 :                 {% with instance as instance_name %}
   18 :                 <p>{% blocktrans %}'{{ instance_name }}' is currently referenced by Following TranslatablePage : {% endblocktrans %}</p>
   19 :                 <ul>
   20 :                     {% for obj in related_pages %}<li><b>{{ obj }} ({{instance_name}})</b></li>{% endfor %}
   21 :                 </ul>
   22 :                 <p>{% blocktrans %} If you delete the language '{{ instance_name }}', All of these pages will also be deleted. {% endblocktrans %}</p>
   23 :                 {% endwith %}
   24 :             {% endif %}
   25 :             <form action="{{ view.delete_url }}" method="POST">

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/core/handlers/exception.py" in inner
  39.             response = get_response(request)

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/core/handlers/base.py" in _get_response
  217.                 response = self.process_exception_by_middleware(e, request)

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/core/handlers/base.py" in _get_response
  215.                 response = response.render()

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/response.py" in render
  109.             self.content = self.rendered_content

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/response.py" in rendered_content
  84.         template = self.resolve_template(self.template_name)

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/response.py" in resolve_template
  66.             return select_template(template, using=self.using)

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/loader.py" in select_template
  48.                 return engine.get_template(template_name)

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/backends/django.py" in get_template
  39.             return Template(self.engine.get_template(template_name), self)

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/engine.py" in get_template
  160.         template, origin = self.find_template(template_name)

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/engine.py" in find_template
  134.                         name, template_dirs=dirs, skip=skip,

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/loaders/base.py" in get_template
  44.                     contents, origin, origin.template_name, self.engine,

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/base.py" in __init__
  191.         self.nodelist = self.compile_nodelist()

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/base.py" in compile_nodelist
  233.             return parser.parse()

File "/Users/j.janssen/.virtualenvs/wagtailtrans/lib/python2.7/site-packages/django/template/base.py" in parse
  518.                     raise self.error(token, e)

Exception Type: TemplateSyntaxError at /admin/wagtailtrans/language/delete/3/
Exception Value: Invalid block tag on line 15: 'get_language_relate_pages', expected 'endif'. Did you forget to register or load this tag?

jjanssen · 2017-08-22T10:58:43Z

src/wagtailtrans/templatetags/wagtailtrans_admin_tags.py

+
+
+@assignment_tag
+def get_relate_pages_for_language(language):


There is a typo in here: get_related_pages_for_language

jjanssen · 2017-09-25T16:53:56Z

Hi @arifin4web

I took a moment to make the overview for deletion a little more appealing. Last thing I noticed when we would be deleting a canonical language it would also be relevant to show the impact of connected pages which are linked to the canonical language.

For eg we have the following languages:

English (default)
Dutch

Deleting English will also delete Dutch. It is this something we can show accordingly in the list of pages we are about to delete?

mikedingjan · 2017-09-27T11:12:04Z

As I remember correctly the canonical_page is a foreign key with models.SET_NULL e.g. when removing a default language it'll remove all pages of that language, and make all the other pages canonical, no more linked pages at all.

Because we have a signal which synchronises the deletion of pages with SYNC_TREE=True this will have other behaviour than expected.

So we can either disconnect the pre_delete signal for this specific use-case. An other option is to disallow the removal of the default language when SYNC_TREE=True and other languages are still in the database.

Thing to keep in mind, LANGUAGES_PER_SITE=True can be tricky with this.

arifin4web · 2018-07-25T06:34:38Z

I apologize for being so late with this MR.

My opinion about removing a default language is that we should never allow them to be deleted. One of the reason being, after we delete a default language we wouldn't be able to create a translatable page. We have to create/set another language as default.

Henk-JanVanHasselaar

@arifin4web can you take a look at issue #10 it would be nice if we can check for the removal of the user groups too.

Henk-JanVanHasselaar · 2018-07-27T07:12:51Z

src/wagtailtrans/views/language.py

+        Also we'll not allow to delete the default language.
+        """
+        if self.instance.is_default:
+            raise Exception("Can't delete a default language")


I would raise a 403 here. instead of 500 error.
Also, maybe it might be nice to check if the language is default before actually doing the post.

Yes agreed. I'll throw a 403 instead.

Well, we don't have to, because in a normal template flow you'll never be in the post endpoint. we have different template so user will not be able to make a post request via admin/dashboard in case of a default language. But this check ensure that even if it does able to make the request we won't be processing this.

Henk-JanVanHasselaar · 2018-07-27T07:14:32Z

src/wagtailtrans/views/language.py

+            raise Exception("Can't delete a default language")
+
+        if self.instance.pages.count():
+            self.instance.pages.all().delete()


You don't have to change this, but why not make the foreign key in TranslatablePage a on_delete=models.CASCADE? I'm curious about your thoughts on that.

As incase of sync=True, on_delete=models.CASCADE will make our tree unstable, I think it's better to handle it here.

mikedingjan requested changes Jul 27, 2017

View reviewed changes

mikedingjan requested changes Jul 28, 2017

View reviewed changes

jjanssen requested changes Aug 22, 2017

View reviewed changes

jjanssen added this to the 0.1.6 milestone Aug 27, 2017

jjanssen added the needs work label Sep 9, 2017

arifin4web force-pushed the feature/remove_language branch from 87bdd28 to 4c29263 Compare July 20, 2018 08:26

arifin4web added needs review and removed needs work labels Jul 25, 2018

Henk-JanVanHasselaar mentioned this pull request Jul 27, 2018

Allow removal of language #10

Open

Henk-JanVanHasselaar suggested changes Jul 27, 2018

View reviewed changes

arifin4web added 2 commits August 9, 2018 08:21

remove a language and associated pages.

c69479f

- disallow the removal of a default language.

19a56bb

arifin4web force-pushed the feature/remove_language branch from 0e79fa9 to 19a56bb Compare August 9, 2018 06:21

arifin4web added 2 commits August 17, 2018 15:47

show forbidden error

dc0ddbc

remove specific translator group/role after removing a language.

e9c79cf

arifin4web force-pushed the feature/remove_language branch from be558a6 to e9c79cf Compare August 20, 2018 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove a Language #100

Remove a Language #100

arifin4web commented Jul 27, 2017

codecov bot commented Jul 27, 2017 •

edited

mikedingjan Jul 27, 2017

mikedingjan Jul 27, 2017

arifin4web Jul 27, 2017

mikedingjan Jul 27, 2017

arifin4web Jul 27, 2017

mikedingjan Jul 27, 2017

mikedingjan Jul 28, 2017

arifin4web Jul 29, 2017

mikedingjan Jul 28, 2017

arifin4web commented Aug 1, 2017

arifin4web commented Aug 9, 2017

jjanssen commented Aug 14, 2017

jjanssen commented Aug 22, 2017 •

edited

jjanssen Aug 22, 2017

arifin4web Aug 23, 2017

jjanssen commented Sep 25, 2017

mikedingjan commented Sep 27, 2017

arifin4web commented Jul 25, 2018

Henk-JanVanHasselaar left a comment

Henk-JanVanHasselaar Jul 27, 2018

arifin4web Jul 27, 2018

Henk-JanVanHasselaar Jul 27, 2018

arifin4web Jul 27, 2018



		@assignment_tag
		def get_language_relate_pages(language):



		@assignment_tag
		def get_relate_pages_for_language(language):

Remove a Language #100

Are you sure you want to change the base?

Remove a Language #100

Conversation

arifin4web commented Jul 27, 2017

codecov bot commented Jul 27, 2017 • edited

Codecov Report

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

arifin4web commented Aug 1, 2017

arifin4web commented Aug 9, 2017

jjanssen commented Aug 14, 2017

jjanssen commented Aug 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjanssen commented Sep 25, 2017

mikedingjan commented Sep 27, 2017

arifin4web commented Jul 25, 2018

Henk-JanVanHasselaar 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

codecov bot commented Jul 27, 2017 •

edited

jjanssen commented Aug 22, 2017 •

edited