Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix #6 #7

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants

I modified the README.md

Owner

skoczen commented Oct 12, 2011

Hey Diego,

Good catch on the readme. However, I'm not convinced that we should be shipping a model with django-ajax-uploader, and less so that south should be required. (Note that I love south, and use it in all of my projects, but think that it's out of the scope of what we're aiming for here.)

To merge the local_database backend, I'd like to see:

  • the models and migrations should have to be explicitly enabled by a setting. Where the uploaded file goes is business logic, and even though you've used GFK's, they're slow, and might not be wanted by users of the lib. We shouldn't be adding tables to user's databases unless they've requested them.
  • No reason to have two migrations, as far as I can tell.
  • Documentation of this backend, particularly of possible schema changes.

Also, if you want to submit those readme changes as a separate pull, I'll merge them right away.

-Steven

Hi Steven,

First, thanks for your answer, I appreciate it.

I make the pull request when the only commit was the README.md, I don't know how or why the others commits appear in the pull request, sorry about that, if you tell me how I can make the pull request for the commit with the changes in the readme file, I will do that immediately.

Now, why I'm shipping a model with the app?, that is because, so many times (2 differents forms and views in a intern project in this moment) is a requirement upload files via ajax and relate the files with models or views, so, I need save a record of file, in that way I can use the code of backends.

My first guess was return the data in the json and then call another view for link the file(s) with my models, but, in that scenario I have two things against, the first, I'm mainly a backend developer, my knowledge of javascript and html, even css and ajax is poor, and second, I figure out is a "bad" idea make the process to upload a file in two steps.

South is not necessary at all, yes, I use migrations, but if the common user doesn't use it too, a simple syncdb will be enough.

You're right about generic relations, but, again, I need the same thing for two models and I think is enough because, I don; t wanna create a model for every kind of model, and in the other side can be used generic relations, but I'm not write that doc in this moment, I think is most a Proof of Concept.

I think in signals too, but I discard that approach, because, maybe use a kind of callback in the backend for give the file to a some method, can end in write the same code many times, but I will like hear your thoughts here.

But definitely, I think we can discuss about a good solution for my problems in particular that can be shared with the world xD.

And again, how can I make the pull request just for the commit of the README.md file?

Owner

skoczen commented Oct 13, 2011

Hey Diego,

No worries on the README.md - github seems to do some magic with pushes after a pull request.

I've been thinking about the best way to implement the scenario you describe. I have a "generic image uploads" (non-ajax) library I use in several projects that does use GFKs, similar to how you've implemented it, and it's worked great for years. So for those local-storage scenarios, it's a reasonable solution.

My concern is for the other backends.

I'd propose this:

  • Make the UploadedFile class a mixin (UploadedFileMixin), and store it in the local_database.py
  • For the models file, check against a django.conf setting, say AJAX_UPLOADER_MODEL_BACKEND.
    • If it's equal to "local_database", then create the model, using the mixin.
    • If it's unset, or has another value, do nothing.

How does that seem to you?

I tossed a few other minor implementation notes on the commit.

@skoczen skoczen commented on the diff Oct 13, 2011

...adedfile_content_type__chg_field_uploadedfile_obje.py
@@ -0,0 +1,44 @@
+# encoding: utf-8
@skoczen

skoczen Oct 13, 2011

Owner

Any reason this can't be a part of the first file?

@diegueus9

diegueus9 Oct 13, 2011

I'm not sure that is south, the migrations was made with the command schemamigration, maybe the option --initial (the first migration) doesn't put the encoding in initial migration.

@skoczen

skoczen Oct 17, 2011

Owner

Interesting. I can live with it as two migrations, just was curious.

@skoczen skoczen and 1 other commented on an outdated diff Oct 13, 2011

ajaxuploader/backends/local_database.py
@@ -0,0 +1,9 @@
+from ajaxuploader.backends.local import LocalUploadBackend as _LocalUploadBackend
+from ajaxuploader.models import UploadedFile
+
+class LocalUploadBackend(_LocalUploadBackend):
+ def upload_complete(self, request, filename):
+ extra_context = super(LocalUploadBackend, self).upload_complete(request, filename)
+ uploaded_file_record = UploadedFile(path=extra_context['path'])
+ uploaded_file_record.save()
@skoczen

skoczen Oct 13, 2011

Owner

I'd return this model instance with the extra_context. Could be useful to someone subclassing.

@diegueus9

diegueus9 Oct 13, 2011

I totally agree with that.

@skoczen skoczen commented on the diff Oct 13, 2011

ajaxuploader/backends/local_database.py
@@ -0,0 +1,9 @@
+from ajaxuploader.backends.local import LocalUploadBackend as _LocalUploadBackend
+from ajaxuploader.models import UploadedFile
+
+class LocalUploadBackend(_LocalUploadBackend):
@skoczen

skoczen Oct 13, 2011

Owner

Would rather see this as LocalDatabaseUploadBackend or LocalModelBasedUploadBackend, instead of the same name, for clarity.

@diegueus9

diegueus9 Oct 13, 2011

Sure, I just don't knew how call this backend.

@skoczen

skoczen Oct 17, 2011

Owner

If we're going to include UploadedFile for all backends, based on AJAX_UPLOADER_USE_MODEL_BACKEND, I'd say actually just put this code in local.py, and wrap it in an if setting.AJAX_UPLOADER_USE_MODEL_BACKEND:

@skoczen skoczen commented on the diff Oct 13, 2011

ajaxuploader/models.py
@@ -0,0 +1,9 @@
+from django.db import models
+from django.contrib.contenttypes.models import ContentType
+from django.contrib.contenttypes import generic
+
+class UploadedFile(models.Model):
+ content_type = models.ForeignKey(ContentType, null=True, blank=True)
+ object_id = models.PositiveIntegerField(null=True, blank=True)
+ content_object = generic.GenericForeignKey('content_type', 'object_id')
+ path = models.CharField(max_length=400)
@skoczen

skoczen Oct 13, 2011

Owner

I could be wrong on this, but don't certain dbs fail if max_length is greater than 255 for a CharField? (Old versions of mysql come to mind)

@diegueus9

diegueus9 Oct 13, 2011

I think so too, that was tested in mysql5.1 but work, maybe that could be a TextField(), if in any point this is integrated with anothers backends, the length of path could be a problem

@skoczen

skoczen Oct 17, 2011

Owner

TextField seems right to me. This isn't going to be an indexed field, and no reason to limit the path length.

Thanks for your feedback, I'm not pretty sure what do you mean with Mixin, like in new GenericViews, can you show me a little snippet of code?

I like your idea of control when create or not the model, but i think, even if I use the s3 backend, I could want save the url of file and relate it with my models, so, I'm trying to do this more generic and not just for the local backend, but maybe we must first work in that and later make it more generic.

in the other hand, in the way to work I was thinking what if the UploadedFile is just an abstract class that must inherit in the process of "installation" of this new backend?

Another requirement in this moment is control the number of files attached to any other model, do you think this could be a common problem, worth think in this or just control it in another side of the project?

Owner

skoczen commented Oct 17, 2011

Hey,

By Mixin, I just mean any general Object-based class, that doesn't inherit from models.Model. This will allow people to mix your class into one of their own models, or enable the built-in model, as they see fit.

I understand your concerns about saving the filename with s3, etc, and I agree - in nearly all cases, users will want to save the file path and name somewhere. Providing a way to save the path, (where path means 'how I can access the file from the app server', not 'how an end user can access the file') seems reasonable, and I can see providing that for all backends. As noted, we'll need to default it off, and document it well.

Propose using AJAX_UPLOADER_USE_MODEL_BACKEND as the setting.

Number of files attached - this seems pretty specific to an app's business logic to me. Do you have an example where you think otherwise?

Owner

skoczen commented Mar 8, 2014

Doing a catch-up clean-out of issues on ajaxuploader. Looks like this one stalled, so I'm closing it. Please re-open if that's not right!

@skoczen skoczen closed this Mar 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment