widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Added some small proofreading nits. Code LGTM as far as I can tell.

Diff comments:

> 
> === modified file 'wlimages/forms.py'
> --- wlimages/forms.py	2013-06-14 19:23:53 +0000
> +++ wlimages/forms.py	2016-11-28 21:56:50 +0000
> @@ -4,12 +4,18 @@
>  import os
>  
>  class UploadImageForm(forms.Form):
> -    imagename = forms.ImageField()
> +    # max_length = 90 because the length of 'upload_to =' has to be considered

Do we still need this line?

> +    imagename = forms.ImageField(max_length=90)
>  
>      def clean_imagename( self ):
> -        name = self.cleaned_data["imagename"]
> -
> -        if Image.objects.has_image(name.name.lower()):
> -            raise forms.ValidationError, "An Image with this name already exists. Please rename and upload again."
> -
> +        in_mem_img = self.cleaned_data["imagename"]
> +        
> +        if Image.objects.has_image(in_mem_img.name):
> +            # Get the existing image to show it's properties

it's (es ist) -> its (sein). Don't worry, native speakers make this mistake a lot too.

> +            exist_img = Image.objects.get(name = in_mem_img.name)
> +            raise forms.ValidationError(
> +                ("An Image with name '%(name)s' is already attached to %(object)s '%(obj_name)s'. \
> +                 Use the existing one or rename your file and upload gain."),

("There is already an image with name '%(name)s' attached to %(object)s '%(obj_name)s'. \
 Use the existing image or rename your file and upload again."),

> +                params={'name': exist_img.name, 'object': exist_img.content_type, 'obj_name': exist_img.content_object}
> +                )
>  
> 
> === modified file 'wlimages/models.py'
> --- wlimages/models.py	2016-02-26 19:00:22 +0000
> +++ wlimages/models.py	2016-11-28 21:56:50 +0000
> @@ -28,25 +27,22 @@
>  
>          if self.filter(name=keyw["name"],revision=keyw["revision"]).count():
>              raise Image.AlreadyExisting()
> -        
> +
>          return super(ImageManager,self).create(**keyw)
>  
> -    def create_and_save_image(self,user,image, content_type, object_id, ip):
> -        # if self.has_image(name):
> -        #     raise RuntimeError,"Image with name %s already exists. This is likely an Error" % name
> -        name = image.name.lower()
> +    def create_and_save_image(self, user, image, content_type, object_id, ip):
> +        # Use Django default's for a safe filename

default's (genitive) -> defaults (plural)

> +        storage = FileSystemStorage()
> +        safe_filename = storage.get_valid_name(image.name)
>          im = self.create(content_type=content_type, object_id=object_id, 
> -                    user=user,revision=1,name=name, editor_ip = ip)
> -
> -        path = "%swlimages/%s" % (MEDIA_ROOT,image.name)
> -        url = "%swlimages/%s" % (MEDIA_URL,image.name)
> +                    user=user, revision=1, name=image.name, editor_ip = ip)
> +        path = "%swlimages/%s" % (MEDIA_ROOT,safe_filename)
>  
>          destination = open(path,"wb")
>          for chunk in image.chunks():
>              destination.write(chunk)
>  
> -        im.image = "wlimages/%s" % (name)
> -        im.url = url
> +        im.image = "wlimages/%s" % (safe_filename)
>  
>          im.save()
>  
> 
> === modified file 'wlimages/views.py'
> --- wlimages/views.py	2016-10-12 20:42:21 +0000
> +++ wlimages/views.py	2016-11-28 21:56:50 +0000
> @@ -30,12 +30,17 @@
>          if form.is_valid(): # All validation rules pass
>              Image.objects.create_and_save_image(user=request.user,image=request.FILES["imagename"],
>                          content_type=ContentType.objects.get(pk=content_type),object_id=object_id, ip=get_real_ip(request))
> -
>              return HttpResponseRedirect(next) # Redirect after POST
>      else:
>          form = UploadImageForm() # An unbound form
> +    
> +    # Get the App (model) to which this image belongs to:
> +    app = ContentType.objects.get(id=content_type)
> +    # Get the current objects name (provided by __unicode__()) from this model

objects -> object's

> +    name = app.get_object_for_this_type(id=object_id)
>  
>      return render_to_response('wlimages/upload.html', {
>          'upload_form': form,
> +        'referer': name,
>      }, context_instance=RequestContext(request))
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands-website/wlimages_rework/+merge/311985
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands-website/wlimages_rework into lp:widelands-website.