knowledgecommonsdc / kcdc3

Django/Python software to run a free school.
http://knowledgecommonsdc.org
MIT License
4 stars 5 forks source link

The "all registrations" reports are returning Nginx 404s #115

Open adavidramos opened 10 years ago

adavidramos commented 10 years ago

We have some pages that return a list of every registration in a session, with information about the registrants. These are slow to render, perhaps because they're database-expensive or because we're doing something dumb.

Anyway, the URLs for these return 404s from Nginx. I think that Django is busily trying to produce the data but Nginx gets tired of waiting and sends the user a 404.

See:

http://knowledgecommonsdc.org/classes/staff/registrations/session/1209/

nyetsche commented 10 years ago

I just checked the nginx configuration, helpfully stored here - https://github.com/knowledgecommonsdc/kcdc3/blob/master/misc/nginx-sites-enabled-kcdc.conf - and I don't see any timeouts explicitly set, which tells me that it should be 60 seconds. http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout

That being said, I:

a) measured it with a wall clock and it times out in 30 seconds b) believe that even if we increase the timeout, it's still "wrong".

Which class/link does this refer to? I need to send you my new SSH keys, right now I can't log into the server.

nyetsche commented 10 years ago

Is it possible that class (or "slug" in the Django parlance) doesn't actually exist? I found this curious artifact in apps/classes/views.py:

# display a list of registrations for a given session
class SessionAdminListView(ListView):

    template_name = "classes/staff_session_list.html"
    context_object_name = "session_list"
    model = Session

    def get_context_data(self, **kwargs):

        context = super(SessionAdminListView, self).get_context_data(**kwargs)
        context['session_list'] = Session.objects.all()

        # is the user staff?
        if self.request.user.is_staff:
            return context
        else:
            # TODO this should really return a 403
            return HttpResponse()

    @method_decorator(login_required)
    def dispatch(self, *args, **kwargs):
        return super(SessionAdminListView, self).dispatch(*args, **kwargs)

It's that "should return a 403" comment that leaves me puzzled. But the melatonin is kicking in so I can't diagnose further tonight.

adavidramos commented 10 years ago

It's possible that the slug doesn't exist.

The "should return a 403" - right now, non-admin users see a blank page. We should instead tell them that they are trespassing.

This issue might not be relevant anymore - see issue #119.

nyetsche commented 10 years ago

Poking at this since I'm bored and cold. So very cold. First, I misidentified the view; based on urls.py it is:

    url(r'^staff/registrations/session/(?P<slug>[A-Za-z0-9_-]+)/$', RegistrationListView.as_view()),
class RegistrationListView(ListView):

    template_name = "classes/staff_registration_list.html"
    context_object_name = "registration_list"
    model = Registration

    def get_context_data(self, **kwargs):

        context = super(RegistrationListView, self).get_context_data(**kwargs)
        context['events'] = Registration.objects.filter(event__session__slug=self.kwargs['slug'])

        # is the user staff?
        if self.request.user.is_staff:
            return context
        else:
            # TODO this should really return a 403
            return HttpResponse()

The SQL statement ultimately being run is

SELECT classes_registration.id, classes_registration.student_id, classes_registration.event_id, classes_registration.date_registered, classes_registration.waitlist, classes_registration.attended, classes_registration.cancelled, classes_registration.date_cancelled, classes_registration.date_promoted, classes_registration.late_promotion FROM classes_registration ORDER BY classes_registration.date_registered ASC;

Which, if I run it manually, returns 8033 rows. In 0.02 seconds. So...not too bad, but clearly more than django can handle, and more than we want.

I'm guessing we want a better filter?

nyetsche commented 10 years ago

I'm thinking now that:

        context['events'] = Registration.objects.filter(event__session__slug=self.kwargs['slug'])

is what's wrong. Specifically event__session__slug=self.kwargs['slug']) - doesn't that ultimately end up as WHERE event.session__slug clause? I don't have a test environment so I'm masochistically computing this in my head (and watching similar runs in an empty sqlite3). The classes_registrations table not have an event, session, or slug field. The classes_session table does has a slug field.

nyetsche commented 10 years ago

I take it back, after going through all of models.py, I do see that there's correctly a Event class, containing a foreign key for Session, containing a foreign key Slug. So maybe it's "all good" but really just just the large query that 404s.

nyetsche commented 10 years ago

Well, 'slug' in the context of Registration object is going to be different than a slug in the Session object than in the Event object. The Session object does has a numerical slug, aka 1409, that maps to what this view is called with. But the slug in Event is a 50-char name.

Maybe that should be filter(event__session_id__slug=self.kwargs['slug'])?

adavidramos commented 10 years ago

No dice for filter(event__session_id__slug=self.kwargs['slug']).

There is a FilteredTeacherAdminListView() method that does filter correctly. I've tried to borrow the same line in RegistrationListView():

event__session_id__slug
event__session_id__slug__iexact
event__session__slug
event__session__slug__iexact

but to no avail.

__iexact just asks for a case-insensitive match, so I don't quite know why we'd need to be using it here anyway.

nyetsche commented 10 years ago

Finally got a VM/vagrant instance running. It does eventually return, it just takes a /really/ long time and I guess our low tier instance is slower than my Mac.

nyetsche commented 10 years ago

Are you sure event__session__slug didn't work? That was my eventual solution, put up here: https://github.com/knowledgecommonsdc/kcdc3/pull/123

I also spent time going through Django docs; an alternate 'solution' I came up with is to add a session foreign key to the Registration model and just work from that. Of course, then we have to manually populate it.

The pull request works on my VM, but there's something "not quite right" with the state of the code - it's pretty difficult to go from a clone to a test environment, even after I manually copied over all the static files.

adavidramos commented 10 years ago

Pretty sure event__session__slug didn't work - but I think you've also changed the context object, which might have been the key.

Will look at this more.

Thanks, Dave

Matt Lesko wrote:

Are you sure |eventsessionslug| didn't work? That was my eventual solution, put up here: #123 https://github.com/knowledgecommonsdc/kcdc3/pull/123

I also spent time going through Django docs; an alternate 'solution' I came up with is to add a session foreign key to the Registration model and just work from that. Of course, then we have to manually populate it.

The pull request works on my VM, but there's something "not quite right" with the state of the code - it's pretty difficult to go from a clone to a test environment, even after I manually copied over all the static files.

— Reply to this email directly or view it on GitHub https://github.com/knowledgecommonsdc/kcdc3/issues/115#issuecomment-32144053.