Merge lp:~widelands-dev/widelands-website/move_minimaps into lp:widelands-website

[kaputtnik]

Move minimaps from 'media/wlmaps/maps/' to 'media/wlmaps/minimaps/' when uploading a map.

Move minimaps from 'media/wlmaps/maps/' to 'media/wlmaps/minimaps/' when uploading a map.

Added a few comments to model wlmaps Map.

If merged only minimaps of newly uploaded maps where moved. I am working also on a branch which moves existing minimaps in media/wlmaps/maps/ to media/wlmaps/minimaps/.

[SirVer (sirver)]

some nits, otherwise lgtm.

review: Approve

[kaputtnik (franku)]

'upload_to' was mandatory before Django 1.7, so it is a leftover of the old Django version. We could omit this option, but it is used to get the property of the url attribute, see the link and explanation below:

https://docs.djangoproject.com/en/1.8/ref/models/fields/#django.db.models.FileField.upload_to

os.path.join is good except for the minimap: The plan is to change this path in a later branch to make it more "Django like": minimap is a file field and such fields have an automatic "url" property. The url property is used in templates, f.e. (second curly braces)

http://bazaar.launchpad.net/~widelands-dev/widelands-website/trunk/view/head:/templates/wlmaps/index.html#L25

-> src="{{ MEDIA_URL }}{{ map.minimap.url }}"

If we omit the leading slash in field minimap (making it "wlmaps/minimaps" instead of "/wlmaps/minimaps/") we could also omit the "{{ MEDIA_URL }}" in the template, because it is then joined with the MEDIA_ROOT setting automatically by Django. As i understand this join is used only if 'upload_to' is set (see the first link above). But i am not sure though... i have to test this :-)

[kaputtnik (franku)]

Just tested: The 'upload_to' property mustn't be set for field 'minimap'. I check this also for the wlmaps.file field.

[kaputtnik (franku)]

I don't know what to do with the 'upload_to' property.

- for the map it is needed
- for the minimap it isn't needed

Defining this property for one file field and not on an other in models.py is imho not good. And at least this is a part of the model. What we can do is:

- Define the upload_to attribute for minimap (like it is)
- get the attributes value in wlmaps.forms.py like: upl_to = Map._meta.get_field('minimap').upload_to
- use 'upl_to' to set the minimap in forms.py

Doing so we would have all the relevant values in models.py, and not one in models.py and one in forms.py. I think this would be better, but i am unsure. What do you think?

[SirVer (sirver)]

sgtm - I agree that having the value once is preferable and the model seems the right place for it. Having a comment there why it is only needed for one of the values should be sufficient.

[kaputtnik (franku)]

I think it would be better if all related code in forms.py depends on 'upload_to', as well for the minimap as also for the mapfile. There are several places where "/wlmaps/maps" or "/wlmaps/minimaps" are hardcoded as strings:

1. Storing the mapfile for wl_map_info
2. Moving the minimap
3. The string for minimap which is stored in the db

If all these places uses the strings of the appropriate 'upload_to' attribute in models.py, the code would be less error-prone, imho. But using the string from 'upload_to' couldn't be implemented in this branch because os.path.join wouldn't work on some places right now:

minimap_path = Map._meta.get_field('minimap').upload_to #="/wlmaps/minimaps"
os.path.join(MEDIA_ROOT, minimaps_path, file.name + '.png' )

In this case MEDIA_ROOT gets omitted because minimap_path has a leading slash -> fail

But my former plan will fix this. In short:
1. apply this branch (moving minimaps from wlmaps/maps/ to wlmaps/minimaps on future uploads)
2. Next branch (correct_old_upload_paths): Contains two admin actions
2.1 Move old minimaps stored in wlmaps/maps to wlmaps/minimaps/ including fixes in the database
2.2.Correct old paths in the database for the maps (f.e. from
    /var/www/django_projects/wlwebsite/code/widelands/media//wlmaps/maps/Gestrandet.wmf
    to
    wlmaps/maps/Gestrandet.wmf
This branch will also change the leading slash for minimaps and maps in the database. As soon these changes are applied the maps page on the website doesn't show the images for the minimaps anymore. But this will be fixed in the next branch.
3. Revert the 2. branch (after the admin actions are executed they are useless)
4. Apply branch "fix_wlimages" (has to be created): Makes the images for maps visible again and change the 'upload_to' to have not the leading slash anymore. If this is done we could also change forms.py to use 'upload_to' in all places where it is needed (see first section in this post)

Yes, i know it looks very complicated, but i haven't found a better solution yet. Of course we could also leave all as it is right now, just because it works... but doing this cleanup may prevent future issues. If one thinks this is useless work, say it now :-)

In case of making it so, we may have to disable map uploading for at least 30 minutes i think.

[lp:~widelands-dev/widelands-website/move_minimaps]

426. By kaputtnik on 2016-08-23

use path.join

[SirVer (sirver)]

> But using the string from 'upload_to' couldn't be implemented in this branch because os.path.join wouldn't work on some places right now.

makes sense - leave a TODO in places where you plan to make changes eventually, but not right now for posterity.

> In case of making it so, we may have to disable map uploading for at least 30 minutes i think.

sounds uncritical to me - we can totally do that.

[Plan of action]

I think your cleanup plan is a good one - cleanups are never a waste of time IMHO, they reduce technical debt. I think the way to describe doing it is feasible - and safe. what I would do instead is using ./manage.py shell and hack in the changes you need to do to the database there - quick example (copy & pasted and edited, I did not change the actual database):

$ ./manage.py shell

In [2]: from wlmaps.models import *

In [3]: maps = Map.objects.all()

In [4]: maps[0]
Out[4]: <Map: Large Ocean by Teayo>

In [5]: maps[0].minimap
Out[5]: <ImageFieldFile: /wlmaps/maps/Large Ocean.wmf.png>

In [6]: maps[0].minimap??
Object `minimap` not found.

In [7]: maps[0].minimap
Out[7]: <ImageFieldFile: /wlmaps/maps/Large Ocean.wmf.png>

In [8]: maps[0].minimap = "/new/path"

In [9]: maps[0].save()

this could be done in a script in a `for map in Map.objects.all()` loop and would therefore not require a change to the source code.

[kaputtnik (franku)]

Thanks for the hint to the shell. Since the code is already written and i am not as familiar with keyboard writing (5 fingers search :-S ) i am in favor of to use a branch.

I would also prefer to make the changes in two steps instead or three. So this branch will become some additional changes and will need then a prerequisite branch. I want this to do after my holydays, in october.

[kaputtnik (franku)]

I've set this to rejected. I think if we allow submitters of maps to upload different versions, we may need a better way of saving the files related to each map (version). E.g. having the wmf and png together in one directory per version would be better.

Unmerged revisions

426. By kaputtnik on 2016-08-23

use path.join

425. By kaputtnik on 2016-08-21

added a few comments

424. By kaputtnik on 2016-08-21

reverted changes to templates

423. By kaputtnik on 2016-08-19

the corrected path is added to the correct_upload_path branch

422. By kaputtnik on 2016-08-17

merged with trunk

421. By kaputtnik on 2016-08-14

only move minimap if no other error occur; fix minimap url things

420. By kaputtnik on 2016-08-14

delete the uploaded file in case of errors

419. By kaputtnik on 2016-08-14

move minimaps to media/wlmaps/minimap/

418. By kaputtnik on 2016-08-14

merged with trunk

=== modified file '.bzrignore'
--- .bzrignore	2016-07-31 11:11:22 +0000
+++ .bzrignore	2016-08-23 20:02:28 +0000
@@ -10,3 +10,4 @@
 media/wlprofile/avatars/*
 media/wlscreens/*
 media/wlimages/*
+media/wlmaps/minimaps/*

=== added directory 'media/wlmaps/minimaps'
=== modified file 'wlmaps/forms.py'
--- wlmaps/forms.py	2016-02-15 14:06:09 +0000
+++ wlmaps/forms.py	2016-08-23 20:02:28 +0000
@@ -11,7 +11,7 @@
 
 from settings import MEDIA_ROOT
 from wlmaps.models import Map
-
+from os import rename, path
 
 class UploadMapForm(ModelForm):
 
@@ -27,18 +27,24 @@
             # no clean file => abort
             return cleaned_data
 
-        name = MEDIA_ROOT + 'wlmaps/maps/' + file.name
+        name = path.join(MEDIA_ROOT, 'wlmaps', 'maps', file.name)
         default_storage.save(name, ContentFile(file.read()))
         try:
             # call map info tool to generate minimap and json info file
             check_call(['wl_map_info', name])
-            # TODO(shevonar): delete file because it will be saved again when
+            
+            # Delete file because it will be saved again when
             # the model is saved. File should not be saved twice
             default_storage.delete(name)
+            
         except CalledProcessError:
             self._errors['file'] = self.error_class(
                 ['The map file could not be processed.'])
             del cleaned_data['file']
+            
+            # The uploaded file should be deleted if an error occurs
+            default_storage.delete(name)
+            
             return cleaned_data
 
         mapinfo = json.load(open(name + '.json'))
@@ -49,6 +55,17 @@
             del cleaned_data['file']
             return cleaned_data
 
+        try:
+            # Move the minimap
+            rename(path.join(MEDIA_ROOT, 'wlmaps', 'maps', file.name + '.png'),
+                   path.join(MEDIA_ROOT, 'wlmaps', 'minimaps', file.name + '.png' ))
+        except OSError:
+            self._errors['file'] = self.error_class(
+                ['The minimap could not be moved. Please inform an admin.'])
+            del cleaned_data['file']
+            
+            return cleaned_data
+
         # Add information to the map
         self.instance.name = mapinfo['name']
         self.instance.author = mapinfo['author']
@@ -60,7 +77,8 @@
 
         self.instance.world_name = mapinfo['world_name']
         # mapinfo["minimap"] is an absolute path and cannot be used.
-        self.instance.minimap = '/wlmaps/maps/' + file.name + '.png'
+        minimap_path = Map._meta.get_field('minimap').upload_to
+        self.instance.minimap = path.join(minimap_path, file.name + '.png')
 
         # the json file is no longer needed
         default_storage.delete(name + '.json')

=== modified file 'wlmaps/migrations/0001_initial.py'
--- wlmaps/migrations/0001_initial.py	2016-08-11 18:13:59 +0000
+++ wlmaps/migrations/0001_initial.py	2016-08-23 20:02:28 +0000
@@ -25,7 +25,7 @@
                 ('nr_players', models.PositiveIntegerField(verbose_name=b'Max Players')),
                 ('descr', models.TextField(verbose_name=b'Description')),
                 ('hint', models.TextField(verbose_name=b'Hint')),
-                ('minimap', models.ImageField(upload_to=b'wlmaps/minimaps', verbose_name=b'Minimap')),
+                ('minimap', models.ImageField(upload_to=b'/wlmaps/minimaps', verbose_name=b'Minimap')),
                 ('file', models.FileField(upload_to=b'wlmaps/maps', verbose_name=b'Mapfile')),
                 ('world_name', models.CharField(max_length=50)),
                 ('pub_date', models.DateTimeField(default=datetime.datetime.now)),

=== modified file 'wlmaps/models.py'
--- wlmaps/models.py	2016-08-11 18:13:59 +0000
+++ wlmaps/models.py	2016-08-23 20:02:28 +0000
@@ -14,6 +14,7 @@
 
 
 class Map(models.Model):
+    # Name of the map given in editor map options
     name = models.CharField(max_length=255, unique=True)
     slug = models.SlugField(unique=True)
     author = models.CharField(max_length=255)
@@ -23,8 +24,13 @@
 
     descr = models.TextField(verbose_name='Description')
     hint = models.TextField(verbose_name='Hint')
+    
+    # Saving of minimap is done in wlmaps.forms.UploadMapForm
+    # The value of 'upload_to' is used over there
     minimap = models.ImageField(
-        verbose_name='Minimap', upload_to='wlmaps/minimaps')
+        verbose_name='Minimap', upload_to='/wlmaps/minimaps')
+    
+    # The real filename of a map
     file = models.FileField(verbose_name='Mapfile',
                             upload_to='wlmaps/maps')