Not sure about `is_teacher` behaviour

[versae]

Taking a look on the code of is_teacher in courses/utils.py, I think that I don't understand what it does. This line has an or when maybe should be an and:

is_teacher = is_teacher or course.teachers.filter(id=user.id).exists()

But, if this is the expected behaviour, there is a fast tweak that can improve the performance, since this function is invoked a lot of times across the code.

def is_teacher(user, courses):
    is_teacher = False
    if user.is_authenticated():
        if isinstance(courses, Course):
            is_teacher = courses.teachers.filter(id=user.id).exists()
        else:
            for course in courses.values("teachers__id").distinct():
                if (course["teachers__id"] == user.id):
                    is_teacher = True
                    break
    return is_teacher

[ablanco]

It's an or because the function checks if the user is a teacher of at least one course, not a teacher of every course.

Why don't you make a pull request with your function?

[versae]

I see. I'd love to, but my code has some changes in models and because there is no migrations, I still have to merge your last changes manually. But I'll do when I had my core ready.

Thanks.

[lorenzogil]

And I don't like we test for authentication in this function. It makes the function less reusable and sometimes I just want to check if a user is a teacher even if it is not the the currently active user.

[versae]

+1

[pitbulk]

I'm implementing a new permission system. https://github.com/OpenMOOC/moocng/blob/improved-api/moocng/courses/permissions.py Hope to finish this work soon. The tastypie API and the main application will use it.

[versae]

I like it! Have you considered the use of django-guardian? Maybe it can help.

[ablanco]

This will be fixed when the new persmissions system is ready.