Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#89 closed enhancement (fixed)

Ability to restrict diskspace per user

Reported by: Bernhard Keller Owned by:
Priority: minor Milestone: 0.6.0
Component: programming Keywords:
Cc: Stephen Compall, joar Parent Tickets:

Description (last modified by Christopher Allan Webber)

for example restrict disk space to 500Mb per user

add user.used\_space =

check prior to upload if space is exceeded
on successful upload add image size to used\_space( check size
after possible resize/conversion )

Do we use the same restriction for every user, or should it be
possible to set a limit for every account ?
possible delete option ? ( what happens to comments )



Change History (18)

comment:1 by Christopher Allan Webber, 13 years ago

I think we should be able to have two restrictions:
- per user
- across the board... if no restriction is explicitly set per user
and there's a global restriction, fallback to the global
restriction



comment:2 by Elrond, 13 years ago

This sounds like post 0.1.0, when a demo site with public
registration is wanted.
I'm quite tempted to rate this "Priority Low".



comment:3 by Christopher Allan Webber, 13 years ago

Agree that it's not high priority, pre 0.1.0 it's not really
needed. But it wouldn't be bad to have either if someone wanted to
work on it. The main thing that's blocking it though is we need to
rethink how files are recorded a bit... I really think we need to
have models.FileRecord objects, whether those are individual
documents each or more likely embedded documents.



comment:4 by Will Kahn-Greene, 13 years ago

The original url for this bug was http://bugs.foocorp.net/issues/375 .

comment:5 by Aleksej, 12 years ago

Component: programming
Type: defectenhancement

comment:6 by Stephen Compall, 12 years ago

Cc: Stephen Compall added

comment:7 by Christopher Allan Webber, 11 years ago

Description: modified (diff)
Owner: Bernhard Keller removed

Removing ownership on this as it hasn't been touched for some time.

comment:8 by rodney757, 11 years ago

Owner: set to rodney757
Status: acceptedin_progress

comment:9 by rodney757, 11 years ago

Owner: rodney757 removed
Status: in_progressreview

Here's a branch for this. https://github.com/rodney757/mediagoblin/tree/upload_limits

Still needs to be tested with the cloud storage, as I can't do that. Also it would be nice to have a client side check of the file size. I'll look into doing that, but I don't have much experience with JS.

comment:10 by Christopher Allan Webber, 11 years ago

Cc: joar added
Milestone: 0.4.1
Owner: set to rodney757
Status: reviewin_progress

Ah, this is really great! This code looks good. I only have two comments:

  • media_confirm_delete decresases the size of uploaded things from request.user, but if the user deleting things is an administrator, you're removing from the wrong user. We should use media.get_uploader instead.
  • Small tyop on:
+                # Get file size an round to 2 decimal places

Everything else looks good. Rodney if you want to make those changes that would be good (if not I can do them), but more importantly, yes, someone needs to test the cloudfiles thing... we can't really merge this until someone does.

Joar, would you be able to help with this? If not, I'll bust out my cloudfiles account next week.

comment:11 by Christopher Allan Webber, 11 years ago

Owner: rodney757 removed
Status: in_progressreview

I'm moving this back to in review.

Really, the comments I made could be done fast... the important thing is that someone test this with cloudfiles. That could be me or someone else.

comment:12 by rodney757, 11 years ago

So #118 was branched from this branch. You previously reviewed them at the same time, so I just made these changes in #118.

Also joar tested it a while ago with cloudfiles and said that everything was working, but it wouldn't hurt to test again.

Sorry about not updating this ticket :)

comment:13 by Christopher Allan Webber, 11 years ago

This seems very close to being mergable... however, the tests are failing, and it looks like a legitimate failure?

=================================== FAILURES ===================================
_____________________ TestSubmission.test_user_under_limit _____________________
self = <mediagoblin.tests.test_submission.TestSubmission instance at 0x37967e8>

    def test_user_under_limit(self):
        self.user_upload_limits(uploaded=499)
    
        # User uploaded should be 499
        assert self.test_user.uploaded == 499
    
        response, context = self.do_post({'title': u'Normal upload 6'},
                                         do_follow=False,
                                         **self.upload_data(MED_PNG))
>       form = context['mediagoblin/submit/start.html']['submit_form']
E       KeyError: 'mediagoblin/submit/start.html'

mediagoblin/tests/test_submission.py:203: KeyError

I think the cause of this should be fixed before merge?

I am going to try to look into it, but some help wouldn't be minded if someone can assist.

comment:14 by Christopher Allan Webber, 11 years ago

It looks like the commit with the first failure is:

commit 6d1108ff8dc2f6d1fa87518b0725515b4ccbc9fe
Author: Rodney Ewing <ewing.rj@gmail.com>
Date:   Thu Jun 13 17:17:33 2013 -0700

    fixed tests and defaults

I'm still going to continue to give the cloudfiles stuff a try. I think the difference is that the behavior expected in the test vs in the view changed. Does that seem right rodney?

We can probably still get this merged for next release if it's done on monday.

comment:15 by Christopher Allan Webber, 11 years ago

So two things:

  1. I just tested it with rackspace cloudfiles. It seems to work just fine.
  2. However it fails to give the proper warning message. It looks like there's a redirect but no message warning the user that they've exceeded their limit, which is I think what we expect. Relatedly, that's what the test is failing on. :)

I think if we can fix that this can get merged....

comment:16 by Christopher Allan Webber, 11 years ago

Milestone: 0.5.00.6.0

comment:17 by Christopher Allan Webber, 11 years ago

Resolution: fixed
Status: reviewclosed

Merged! :D

comment:18 by Christopher Allan Webber, 11 years ago

(Relatedly, in case you wondered: I fixed the tests.)

Note: See TracTickets for help on using tickets.