3
\$\begingroup\$

I've been working on probably my biggest project yet, and I'd like you guys to give me some feedback on the code I wrote so far.

It's an eLearning app which offers users a host of functionalities to get better at school subjects, basically. It's centered around mockup tests made up of multiple choice questions added by teachers.

Here's the repository: https://github.com/samul-1/elearning There, you'll find a more in-depth explanation of the features I'm working on.

The project is already fully functional, but I'm working on polishing a few things. The backend is ran by Django, and the frontend is made with Vue.js. I've been at both of these frameworks for around 6-7 months now, so you could say I'm just starting out.

Personally, I'm very proud of what I've been able to build so far, but I constantly strive to get better, so I'd like some feedback on these points:

  • code best practices. Is my code well written, legible, and clear?
  • software architecture. Does my choice of models, views, and data validation make sense?
  • (this might be slightly off-topic so you can skip it) does the user interface look good? What could be done to improve it?

I hope the question isn't too generic as it is right now. If in fact it is, just let me know and I'll edit it to ask about more specific aspects.

What I can say already, is that I'm mostly interested in feedback about these files:

elearningapp/models.py
elearningapp/views.py
elearningapp/forms.py
users/models.py

and the vue components at elearningapp/vue_frontend/src/components (these might be a sore spot).

The core of this project can probably be boiled down to the models and views of the elearningapp Django app, so I'll share the code for those two files.

elearningapp/models.py

# imports

class Course(models.Model):
    name = models.CharField(max_length=100)
    number_of_questions_per_test = models.IntegerField(default=10)
    # True if the course has a category distribution, i.e. tests choose a fixed amount of questions from each category
    uses_category_distribution = models.BooleanField(default=False)
    points_for_correct_answer = models.FloatField(default=1)
    points_for_unanswered = models.FloatField(default=0)
    points_for_wrong_answer = models.FloatField(default=-0.5)
    minimum_passing_score = models.FloatField(default=5)

    def __str__(self):
        return self.name

    # returns 'amount' random questions that user hasn't seen yet; if category is specified,
    # the questions will be from that category
    # raises OutOfQuestionsException if there aren't enough questions that satisfy requirements
    def get_questions_for(self, user, amount, category=None):
        # get list of questions already seen by user
        seen_questions = SeenQuestion.objects.filter(user=user)
        seen_question_ids = map(lambda q: q.question.pk, seen_questions)

        questions = Question.objects.filter(course=self).exclude(
            id__in=seen_question_ids
        )
        if category:
            questions = questions.filter(category=category)

        # pick random questions
        try:
            random_questions = random.sample(list(questions), amount)
        except ValueError as err:  # if there aren't enough questions left, return None
            raise OutOfQuestionsException

        return random_questions

    # returns an object containing info about the course
    def get_aggregated_info(self):
        subscribers = apps.get_model(
            "users", model_name="CourseSpecificProfile"
        ).objects.filter(course=self)
        taken_tests = TakenTest.objects.filter(course=self)

        avg_score = (
            (sum([test.score for test in list(taken_tests)]) / taken_tests.count())
            if taken_tests.count() > 0
            else 0
        )

        return {
            "number_of_subscribers": subscribers.count(),
            "number_of_tests_taken": taken_tests.count(),
            "average_score": round(avg_score, 1),
        }

    # returns 'amount' questions, showing correct answer and solution too
    # (meant for use inside of course control panel)
    def get_complete_questions(self, amount, pk_greater_than=0, category=None):
        questions = self.question_set.filter(pk__gt=pk_greater_than)
        if category is not None:
            cat = Category.objects.get(pk=category)
            questions = questions.filter(category=cat)

        questions = questions.order_by("pk")[:amount]
        # TODO add sorting
        return list(
            map(
                lambda q: q.format_complete_question(),
                questions,
            )
        )

    # ? move this to CourseSpecificProfile
    def get_seen_questions(self, user, amount, pk_greater_than=0, category=None):
        questions = user.seenquestion_set.filter(pk__gt=pk_greater_than)
        if category is not None:
            cat = Category.objects.get(pk=category)
            questions = questions.filter(category=cat)

        questions = questions.order_by("pk")[:amount]
        # TODO add sorting
        return list(
            map(
                lambda q: q.serialize(),
                questions,
            )
        )

    # returns the 'quantity' hardest questions from this course,
    # i.e. those with the lowest percentage of times they were answered correctly
    def get_hardest_questions(self, quantity):
        return list(
            map(
                lambda q: q.format_complete_question(),
                self.question_set.all().order_by("percentage_of_correct_answers")[
                    :quantity
                ],
            )
        )

    # returns the last 'quantity' actions taken by course admins or collaborators
    def get_last_actions(self, quantity):
        return list(
            map(
                lambda a: a.serialize(),
                self.staffaction_set.all().order_by("-timestamp")[:quantity],
            )
        )

    # returns the profiles of users who are subscribed to this course
    def get_subscribed_users(self):
        return list(map(lambda u: u.serialize(), self.coursespecificprofile_set.all()))

    # returns all the reports that have been made to questions from this course
    # if resolved is specified, only reports with that status are returned
    def get_reports(self, resolved=None):
        reports = Report.objects.filter(question__course=self)

        if resolved is not None:
            reports = reports.filter(resolved=resolved)
        return list(map(lambda r: r.serialize(), reports))

    def maximum_score(self):
        return self.points_for_correct_answer * self.number_of_questions_per_test


# used to keep track of courses assistants' permissions
# (it's meant to work kinda like Django built-in permission system, but on a per-instance basis rather than per-model)
class CoursePermission(models.Model):
    user = models.OneToOneField("users.CourseSpecificProfile", on_delete=models.CASCADE)
    # course = models.ForeignKey(Course, on_delete=models.CASCADE)
    can_add_questions = models.BooleanField(default=True)
    can_edit_questions = models.BooleanField(default=True)
    can_manage_contributors = models.BooleanField(default=False)
    # can_edit_contributors = models.BooleanField(default=False)

    def serialize(self):
        return {
            "can_add_questions": self.can_add_questions,
            "can_edit_questions": self.can_edit_questions,
            "can_manage_contributors": self.can_manage_contributors,
        }


class Category(models.Model):
    course = models.ForeignKey(
        Course, on_delete=models.CASCADE, related_name="categories"
    )  # many to one
    name = models.CharField(max_length=100)
    # how many questions from this category need to appear in each test of this course
    # (used only if the course has 'category distribution' enabled)
    quantity = models.PositiveIntegerField(default=None, null=True)

    def __str__(self):
        return self.name


class Question(models.Model):
    course = models.ForeignKey(Course, on_delete=models.CASCADE)  # many to one
    category = models.ForeignKey(
        Category, null=True, blank=True, default=None, on_delete=models.SET_NULL
    )  # many to one
    rendered_text = (
        models.TextField()
    )  # contains public text including html generated by mathjax
    text = models.TextField(
        default=""
    )  # contains the actual test that was input upon creating the question
    correct_answer_index = models.PositiveIntegerField()
    solution_text_rendered = models.TextField()
    solution_text = models.TextField(default="")
    # TODO add logic to track who added a question
    added_by = models.ForeignKey(User, null=True, on_delete=models.SET_NULL)
    number_of_appearances = models.PositiveIntegerField(default=0)
    percentage_of_correct_answers = models.FloatField(default=100.0)

    def __str__(self):
        return self.text

    # when model is saved, process the question text to render TeX as svg
    def save(self, re_render_text=True, *args, **kwargs):
        # TODO raise exception if question is saved to a category for a course different than that specified in fk Course

        if re_render_text:
            self.rendered_text = tex_to_svg(self.text)
            self.solution_text_rendered = tex_to_svg(self.solution_text)

        return super(Question, self).save(*args, **kwargs)

    # returns a dict containing the public information about the question;
    # i.e. its text and the text of its answers
    def format_question_for_user(self):
        output = {}
        output["text"] = self.rendered_text

        # get all answers to the question
        answers = self.answer_set.all()  # Answer.objects.filter(question=self)
        output["answers"] = [a.rendered_text for a in list(answers)]
        
        return output

    # returns a dict containing all the information about the question;
    # i.e. all its public information, the index of the correct answer, and the solution,
    # as well as the source code for all the texts (question, solution, answers), used for editing a question
    def format_complete_question(self):
        info = self.format_question_for_user()
        info["textSource"] = self.text
        info["solution"] = self.solution_text_rendered
        info["solutionSource"] = self.solution_text
        info["correctAnswerIndex"] = self.correct_answer_index
        info["questionId"] = self.pk
        info["category"] = self.category.pk
        info["wrongAnswersPercentage"] = 100 - self.percentage_of_correct_answers

        # get the source text for all the answers
        answers_sources = Answer.objects.filter(question=self)
        info["answersSources"] = [a.text for a in list(answers_sources)]
        return info

    # returns the PERCENTAGE of times this question was answered correctly relative to
    # how many times it appeared in tests
    # this is intended to be called ONLY by Answer.save() each time this question is answered
    # to access this property from somewhere else, use the field percentage_of_correct_answers
    def get_percentage_right_answers(self):
        right_answer = self.answer_set.get(answer_index=self.correct_answer_index)
        if self.number_of_appearances == 0:
            return 100
        return right_answer.selections / self.number_of_appearances * 100


# a report made by a user about a question containing a mistake
class Report(models.Model):
    timestamp = models.DateTimeField(auto_now=True)
    user = models.ForeignKey(
        settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True
    )
    question = models.ForeignKey(Question, on_delete=models.CASCADE, null=True)
    text = models.TextField(default="")
    resolved = models.BooleanField(default=False)

    def serialize(self):
        return {
            "reportId": self.pk,
            "timestamp": str(self.timestamp),
            "userId": self.user.pk,
            "username": self.user.username,
            "firstName": self.user.first_name,
            "lastName": self.user.last_name,
            "question": self.question.format_complete_question(),
            "text": self.text,
            "resolved": 1 if self.resolved else 0,
        }


class Answer(models.Model):
    question = models.ForeignKey(Question, on_delete=models.CASCADE)  # many to one
    rendered_text = models.TextField()
    text = models.TextField(default="")
    answer_index = models.PositiveIntegerField()
    selections = models.PositiveIntegerField(default=0)

    def __str__(self):
        return self.text

    # when model is saved, process the question text to render TeX as svg
    # and update the percentage of times the corresponding question was answered correctly
    def save(self, re_render_text=True, *args, **kwargs):
        if re_render_text:
            self.rendered_text = tex_to_svg(self.text)

        instance = super(Answer, self).save(*args, **kwargs)

        # re-compute the percentage of correct answers to the question
        self.question.percentage_of_correct_answers = (
            self.question.get_percentage_right_answers()
        )
        self.question.save(re_render_text=False)

        return instance


"""
Models to manage history, active tests, and course cp logs
"""


class StaffAction(models.Model):
    ACTIONS = [
        ("C", "Create"),
        ("E", "Edit"),
    ]
    course = models.ForeignKey(Course, on_delete=models.CASCADE)  # many to one
    action = models.CharField(max_length=1, choices=ACTIONS)
    question = models.ForeignKey(Question, on_delete=models.CASCADE)  # many to one
    user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE)
    timestamp = models.DateTimeField(auto_now=True)

    def __str__(self):
        return (
            str(self.course)
            + ": "
            + str(self.user)
            + " "
            + str(self.action)
            + " "
            + str(self.question)
        )

    def serialize(self):
        return {
            "action": self.action,
            "user": self.user.username,
            "question": self.question.text,
            "questionId": self.question.pk,
            "timestamp": str(self.timestamp),
        }


# a test that was taken by a user, detailed with its outcome
class TakenTest(models.Model):
    user = models.ForeignKey(User, on_delete=models.CASCADE)  # many to one
    course = models.ForeignKey(
        Course, on_delete=models.CASCADE, null=True
    )  # many to one
    timestamp = models.DateTimeField(auto_now=True)
    score = models.FloatField()
    passing = models.IntegerField(default=0)  # boolean?

    # returns a json representation of self that the client can consume
    def serialize(self):
        json_self = {
            "score": self.score,
            "timestamp": str(self.timestamp),
            "correctlyAnsweredQuestions": [],
            "unansweredQuestions": [],
            "incorrectlyAnsweredQuestions": [],
            "passing": self.passing,
        }

        for answer in AnswersInTakenTest.objects.filter(test=self):
            # get question info and add the answer that was given to this question in the test
            question_with_your_answer = answer.question.format_complete_question()
            question_with_your_answer["yourAnswer"] = answer.answer_index

            # append question to corresponding list based on whether it was answered correctly,
            # incorrectly, or left unanswered
            if answer.answer_index == -1:
                json_self["unansweredQuestions"].append(question_with_your_answer)
            elif answer.answer_index == answer.question.correct_answer_index:
                json_self["correctlyAnsweredQuestions"].append(
                    question_with_your_answer
                )
            else:
                json_self["incorrectlyAnsweredQuestions"].append(
                    question_with_your_answer
                )

        return json_self


# a question that appeared on a TakenTest, and its answer
class AnswersInTakenTest(models.Model):
    test = models.ForeignKey(TakenTest, on_delete=models.CASCADE)
    question = models.ForeignKey(Question, on_delete=models.CASCADE)
    # ? should probably use null instead of -1 for unanswered
    answer_index = models.IntegerField()  # -1 if unanswered


# a question that was seen by the user, together with its answer
# needs a separate model from TakenTest because the history of seen questions
# is erasable, whereas that of taken tests isn't
class SeenQuestion(models.Model):
    user = models.ForeignKey(User, on_delete=models.CASCADE)
    question = models.ForeignKey(Question, null=True, on_delete=models.CASCADE)
    # ? should probably use null instead of -1 for unanswered
    answer_index = models.IntegerField(default=-1)  # -1 if unanswered
    timestamp = models.DateTimeField(auto_now_add=True)

    def serialize(self):
        return {
            "questionId": self.pk,
            "question": self.question.format_complete_question(),
            "givenAnswer": self.answer_index,
        }


# a test currently, associated to a user: used in case they leave and come back to the test
# to retrieve the data without generating a new one, as well as to keep track of what questions
# the answers given by the user need to be checked against
class ActiveTest(models.Model):
    user = models.ForeignKey(User, on_delete=models.CASCADE)
    questions = models.ManyToManyField(Question)
    course = models.ForeignKey(Course, on_delete=models.CASCADE)
    timestamp = models.DateTimeField(auto_now_add=True)

    # chooses random questions that the requesting user hasn't seen yet according to the course distribution (if any)
    # and adds them to the ManyToMany relationship with Question
    def init_test(self):
        if not self.course.uses_category_distribution:
            # course doesn't have a category distribution (or questions for this course aren't grouped in categories)
            for question in self.course.get_questions_for(
                self.user, self.course.number_of_questions_per_test
            ):
                self.questions.add(question)
        else:
            chosen_questions = []
            # get the right number of questions for each category
            for category in self.course.categories.all():
                chosen_questions.extend(
                    list(
                        self.course.get_questions_for(
                            self.user, category.quantity, category
                        )
                    )
                )

            random.shuffle(chosen_questions)
            for question in chosen_questions:
                self.questions.add(question)
        self.save()

    # returns a list containing all questions of the test,
    # formatted to display their public information
    def format_test_for_user(self):
        output = []

        i = 1

        for question in self.questions.all():
            # add question index to dict for displaying to the user
            temp = question.format_question_for_user()
            temp["idx"] = i
            output.append(temp)
            i += 1

        return output

    # takes in a dict containing the answers to the test's questions; computes the result,
    # updates data about questions and answers, saves the test questions to the history of
    # seen questions, and returns a TakenTest object detailing the outcome of the test
    def evaluate_answers(self, answers):
        taken_test = TakenTest(user=self.user, course=self.course, score=0)
        taken_test.save()

        # update the number of tests taken by the user
        user_profile = self.user.coursespecificprofile_set.get(course=self.course)
        user_profile.number_of_tests_taken += 1
        user_profile.save()

        questions = self.questions.all()  # ? could use select_related('answer_set')
        score = 0

        for question, answer in zip(
            questions, map(lambda a: answers[a], answers)
        ):  # map {index:answer} to answer

            # increment number of appearances of this question
            question.number_of_appearances += 1
            question.save(re_render_text=False)
            # increment number of selections for this answer
            if answer != -1:
                given_answer = question.answer_set.get(answer_index=answer)
                print(given_answer)
                given_answer.selections += 1
                given_answer.save(re_render_text=False)

            # record given answer for history
            ans = AnswersInTakenTest(
                answer_index=answer, question=question, test=taken_test
            )
            ans.save()

            # record question for (deletable) history
            seen_question = SeenQuestion(
                user=self.user, question=question, answer_index=answer
            )
            seen_question.save()

            if int(answer) == question.correct_answer_index:
                score += self.course.points_for_correct_answer
            elif int(answer) == -1:
                score += self.course.points_for_unanswered
            else:
                score += self.course.points_for_wrong_answer

        if score > self.course.minimum_passing_score:
            taken_test.passing = 1  # using 1, 0 instead of True, False to make it easier to convert to JSON
        else:
            taken_test.passing = 0

        taken_test.score = score
        taken_test.save()
        user_profile.last_score = score
        user_profile.save()

        return taken_test

elearningapp/views.py

# imports

@login_required
def create_course(request):
    if not request.user.globalprofile.is_teacher:
        return HttpResponseForbidden()

    if request.method == "POST":
        form_data = json.loads(request.body.decode("utf-8"))
        form = CourseForm(form_data, user=request.user.globalprofile)
        print(form_data)
        if form.is_valid():
            new_course = form.save()
            CourseSpecificProfile.objects.create(user=request.user, course=new_course)
        else:
            print(form.errors)

        return JsonResponse({"courseId": new_course.pk}, safe=False)

    # GET
    return render(
        request,
        "elearningapp/createcourse.html",
    )


# course control panel
@login_required
def course_cp(request, course_id):
    course = get_object_or_404(Course, pk__exact=course_id)

    # reject with 403 if user isn't authorized to view the control panel for this course
    if course not in request.user.globalprofile.admin_of.all() and (
        request.user.coursespecificprofile_set.filter(course=course).count == 0
        or not hasattr(
            request.user.coursespecificprofile_set.get(course=course),
            "coursepermission",
        )
    ):
        return HttpResponseForbidden()

    aggregated_info = course.get_aggregated_info()

    context = {
        "course": course,
        "reports": course.get_reports(resolved=False),
        "last_actions": course.get_last_actions(5),
        "number_of_subscribers": aggregated_info["number_of_subscribers"],
        "number_of_tests_taken": aggregated_info["number_of_tests_taken"],
        "average_score": aggregated_info["average_score"],
        "hardest_questions": course.get_hardest_questions(3),
        "admin": "true"
        if course in request.user.globalprofile.admin_of.all()
        else "false",  # using 'true' and 'false' to prevent issues with js frontend consuming the value
    }

    # get user's permissions, if they have a permission object associated to them (admins don't)
    if len(
        course_profile := request.user.coursespecificprofile_set.filter(course=course)
    ) != 0 and hasattr(course_profile[0], "coursepermission"):
        context["my_permissions"] = json.dumps(
            course_profile[0].coursepermission.serialize()
        )
        context["user_id"] = course_profile[0].pk
    else:
        context["my_permissions"] = {}
        context[
            "user_id"
        ] = "null"  # once again using 'null' as a string for easier passing of the value as a prop

    return render(
        request,
        "elearningapp/course_cp.html",
        context,
    )


# accessed via GET, returns a list of users subscribed to the course
@login_required
def get_course_users(request, course_id):
    course = get_object_or_404(Course, pk__exact=course_id)
    return JsonResponse(course.get_subscribed_users(), safe=False)


# accessed via GET, returns a lit of reports that have been made to questions from the course
@login_required
def get_course_reports(request, course_id):
    course = get_object_or_404(Course, pk__exact=course_id)
    return JsonResponse(course.get_reports(), safe=False)


# if accessed via GET, gets the first 5 questions for the course and renders template containing the EditQuestion component
# if accessed via PUT, updates the question
# if question_id is specified, the id is passed via the context object to EditQuestion vue component
@login_required
def edit_question(request, course_id, question_id=None):
    course = get_object_or_404(Course, pk__exact=course_id)

    # reject with 403 if user isn't authorized to edit questions for this course
    if course not in request.user.globalprofile.admin_of.all() and (
        request.user.coursespecificprofile_set.filter(course=course).count == 0
        or not hasattr(
            request.user.coursespecificprofile_set.get(course=course),
            "coursepermission",
        )
        or not request.user.coursespecificprofile_set.get(
            course=course
        ).coursepermission.can_edit_questions
    ):
        return HttpResponseForbidden()

    if request.method == "PUT":
        form_data = json.loads(request.body.decode("utf-8"))
        question = get_object_or_404(Question, pk=form_data["questionId"])
        form = QuestionForm(form_data, user=request.user, action="E", instance=question)

        print(form_data)
        if form.is_valid():
            print("is valid")
            updated_question = form.save()
            print(updated_question)
            return JsonResponse(updated_question.format_complete_question(), safe=False)
        else:
            print(form.errors)

    # GET
    categories = Category.objects.filter(course=course)
    questions = course.get_complete_questions(5)

    context = {
        "course_id": course_id,
        "categories": [{c.pk: c.name} for c in categories],
        "questions": questions,
    }

    if question_id is not None:
        context["editing_id"] = question_id

    return render(
        request,
        "elearningapp/edit_question.html",
        context,
    )


# if accessed via POST, creates a new report for the specified question
# if accessed via PUT, updates the status of the specified report
@login_required
def report_question(request):
    if request.method != "POST" and request.method != "PUT":
        return HttpResponseNotAllowed()

    form_data = json.loads(request.body.decode("utf-8"))

    if request.method == "POST":
        question = get_object_or_404(Question, pk__exact=form_data["questionId"])
        form = ReportForm(form_data, user=request.user, question=question)

        if form.is_valid():
            # add new report to db
            form.save()

            return JsonResponse({"success": True})
        else:
            print(form.errors)

    if request.method == "PUT":
        report = get_object_or_404(Report, pk__exact=form_data["reportId"])
        # add report text to form data as it is a mandatory field that isn't supplied when the view is accessed via PUT
        # ? maybe there's a better way to do this
        form_data["text"] = report.text

        form = ReportForm(form_data, instance=report, resolved=form_data["resolved"])
        print(form_data)
        if form.is_valid():
            print("is valid")
            form.save()
            return JsonResponse({"success": True})
        else:
            print(form.errors)


# if accessed via PUT, updates or creates the permissions of a user for a course,
# if accessed via DELETE, deletes the permission object for the specified user and course
def update_course_permissions(request, course_id):
    if request.method == "GET":
        return HttpResponseNotAllowed()
    course = get_object_or_404(Course, pk__exact=course_id)

    # reject with 403 if user isn't authorized to add assistants for this course
    if course not in request.user.globalprofile.admin_of.all() and (
        request.user.coursespecificprofile_set.filter(course=course).count == 0
        or not hasattr(
            request.user.coursespecificprofile_set.get(course=course),
            "coursepermission",
        )
        or not request.user.coursespecificprofile_set.get(
            course=course
        ).coursepermission.can_manage_contributors
    ):
        return HttpResponseForbidden()

    form_data = json.loads(request.body.decode("utf-8"))
    print(form_data)
    editing_profile = get_object_or_404(
        CourseSpecificProfile, pk=form_data["profile_id"]
    )
    if request.method == "PUT":
        # cannot edit permissions of a course admin
        if course in editing_profile.user.globalprofile.admin_of.all():
            return HttpResponseForbidden()
        # retrieve or create permissions for this user
        # we don't need the boolean returned by get_or_create, hence the _ wildcard
        permissions, _ = CoursePermission.objects.get_or_create(user=editing_profile)
        form = PermissionForm(form_data["permissions"], instance=permissions)

        print(form_data)
        if form.is_valid():
            form.save()
            return JsonResponse({"success": True})
        else:
            print(form.errors)

    if request.method == "DELETE":
        try:
            permissions = CoursePermission.objects.get(user=editing_profile)
        except CoursePermission.DoesNotExist:
            return HttpResponseNotFound()
        permissions.delete()
        return JsonResponse({"success": True})


# accessed via GET by the client for infinite scrolling in the EditQuestion vue component
@login_required
def get_questions(request, course_id, amount, starting_from_pk, category=None):
    course = get_object_or_404(Course, pk__exact=course_id)
    questions = course.get_complete_questions(
        int(amount), pk_greater_than=int(starting_from_pk), category=category
    )
    return JsonResponse(questions, safe=False)


# accessed via GET by the client for infinite scrolling in the QuestionHistory vue component
@login_required
def get_seen_questions(request, course_id, amount, starting_from_pk, category=None):
    course = get_object_or_404(Course, pk__exact=course_id)
    questions = course.get_seen_questions(
        request.user,
        int(amount),
        pk_greater_than=int(starting_from_pk),
        category=category,
    )
    print(questions)
    return JsonResponse(questions, safe=False)


# renders template containing CreateQuestion vue component when accessed via GET,
# handles question creation using Question ModelForm when accessed via POST
@login_required
def add_question(request, course_id):
    course = get_object_or_404(Course, pk__exact=course_id)
    # reject with 403 if user isn't authorized to add questions to this course
    if course not in request.user.globalprofile.admin_of.all() and (
        request.user.coursespecificprofile_set.filter(course=course).count == 0
        or not hasattr(
            request.user.coursespecificprofile_set.get(course=course),
            "coursepermission",
        )
        or not request.user.coursespecificprofile_set.get(
            course=course
        ).coursepermission.can_add_questions
    ):
        return HttpResponseForbidden()

    categories = Category.objects.filter(course=course)

    context = {
        "course_id": course_id,
        "categories": [{c.pk: c.name} for c in categories],
    }
    if request.method == "POST":
        form_data = json.loads(request.body.decode("utf-8"))
        form = QuestionForm(form_data, user=request.user, action="C")

        if form.is_valid():
            # add new question to db
            new_question = form.save()

            return JsonResponse("ok", safe=False)
        else:
            print(form.errors)
        return JsonResponse({"success": True})

    # GET
    return render(
        request,
        "elearningapp/add_question.html",
        context,
    )


# renders a test for the user
@login_required
def render_test(request, course_id):
    requesting_user = request.user  # User.objects.get(pk=user_id)
    course = Course.objects.get(pk=course_id)

    if ActiveTest.objects.filter(user=requesting_user, course=course).count() > 0:
        # user already has an active test associated to them; use that one
        # instead of creating a new one
        current_test = ActiveTest.objects.get(user=requesting_user, course=course)
    else:
        # no active tests found for this user;
        # create a new test associated to requesting user in selected course
        try:
            current_test = ActiveTest(user=requesting_user, course=course)
            current_test.save()
            current_test.init_test()
        except OutOfQuestionsException:
            # delete the test that was attempted to be initialized if exception occurs
            current_test.delete()
            # TODO show an actual page
            return HttpResponse("out of questions")

    # get user's global data
    global_profile = GlobalProfile.objects.get(user=request.user)
    global_user_data = {
        "name": global_profile.user.username,
        "id": global_profile.user.pk,
    }

    context = {
        "course_id": course.pk,
        "questions": current_test.format_test_for_user(),
        "global_user_data": global_user_data,
    }

    return render(request, "elearningapp/test.html", context)


# returns the list of questions that the user has seen in past tests
@login_required
def question_history(request, course_id):
    user_profile = get_object_or_404(
        CourseSpecificProfile, user__exact=request.user, course__pk__exact=course_id
    )

    course = Course.objects.get(pk=course_id)
    seen_questions = course.get_seen_questions(user=request.user, amount=5)

    # seen_questions = map(
    #     lambda sq: sq.serialize(), list(user_profile.get_seen_questions())
    # )
    return render(
        request,
        "elearningapp/question_history.html",
        {
            "questions": list(seen_questions),
            "user_id": request.user.pk,
            "course_id": course_id,
        },
    )


# empties the list of seen question for given user and course
@login_required
def delete_question_history(request, course_id):
    user_profile = get_object_or_404(
        CourseSpecificProfile, user__exact=request.user, course__pk__exact=course_id
    )

    for question in user_profile.get_seen_questions():
        question.delete()

    return JsonResponse("ok", safe=False)


# returns the list of tests that the user has taken in the past
@login_required
def test_history(request, course_id):
    user_profile = get_object_or_404(
        CourseSpecificProfile, user__exact=request.user, course__pk__exact=course_id
    )

    taken_tests = map(lambda t: t.serialize(), list(user_profile.get_taken_tests()))
    return render(
        request,
        "elearningapp/test_history.html",
        {
            "tests": list(taken_tests),
            "maxScore": Course.objects.get(pk=course_id).maximum_score(),
        },
    )


# calls a method to evaluate the answers given, save the question and test outcome to user's history;
# returns details about the outcome of the test
@login_required
def check_answers(request):
    if request.method != "POST":
        return HttpResponseNotAllowed()

    answers = json.loads(request.body.decode("utf-8"))
    print(answers)
    # TODO check validity of sent json object
    requesting_user = request.user

    # ! TODO check course in addition to requesting user
    current_test = ActiveTest.objects.get(user=requesting_user)

    outcome = current_test.evaluate_answers(answers)
    # current_test.delete()

    return JsonResponse(outcome.serialize())


# retrieves context for rendering the course dashboard
@login_required
def view_course(request, course_id):
    course = get_object_or_404(Course, pk=course_id)
    # get course's data
    course_data = {
        "name": course.name,
        "id": course.pk,
    }

    # get user's global data
    global_profile = GlobalProfile.objects.get(
        user=request.user
    )  # we can assume this exists because it's created at signup time and the view has @login_required
    global_user_data = {
        "name": global_profile.user.first_name
        if global_profile.user.first_name != ""
        else global_profile.user.username,
        "id": global_profile.user.pk,
    }

    try:
        course_profile = CourseSpecificProfile.objects.get(
            user=request.user, course__pk=course_id
        )
    except CourseSpecificProfile.DoesNotExist:
        # user isn't signed up to this course, give them a chance to
        return render(
            request,
            "course_register.html",
            {
                "global_user_data": global_user_data,
                "course_data": course_data,
            },
        )

    # get user's course specific data
    course_specific_user_data = {
        "number_of_tests_taken": course_profile.number_of_tests_taken,
        "last_score": course_profile.last_score,
        "average_score": round(course_profile.get_average_score(), 1),
        "last_scores": course_profile.get_last_scores(5),
    }

    return render(
        request,
        "elearningapp/course_dashboard.html",
        {
            "global_user_data": global_user_data,
            "course_specific_user_data": course_specific_user_data,
            "course_data": course_data,
        },
    )


And here's a brief explanation of what the app does: teachers can sign up and create courses, to which they can add questions and categories to divide the questions into. Students can then sign up to courses and take tests made up of randomly chosen questions among those in the course. After the test is over, the questions are saved to the user's personal history (an erasable history) and don't show up again (unless the history is cleared), and the results of the tests are saved to a separate (persistent) history to check progress over time. There's a whole host of additional features like a course control panel for teachers to track stats about their course, add assistants, and so on, as well as the possibility for users to report errors in questions.

I really love doing this and I want to be as good at it as I can. So thank you to anybody who will take time to review my code.

\$\endgroup\$
1
  • \$\begingroup\$ Well this is whole lot of review you are asking. It would be easier and faster more efficient to do it if you present a portion of the code base like one of the class or some dependent classes for system design and architectural decisions review. And I recommend to make those reviews separate as code review and system design reviews are two separate parts. The more specific we are, better review we can get. I have raised some issues in your project after just skimming through the README and code. I hope it helps. \$\endgroup\$ Feb 20 at 15:14
1
\$\begingroup\$

Code Review - elearningapp/models.py - Course

FYKI - Im not a Python expert!

Anit's recommendation are marked like this!

Well the first thing I would do is make separate files for the classes.

class Course(models.Model):
    name = models.CharField(max_length=100)
    number_of_questions_per_test = models.IntegerField(default=10)

Lets remove comments as such! The code should be good enough to do it. # True if the course has a category distribution, i.e. tests choose a fixed amount of questions from each category

I'm not against snake_case but I would prefer CamelCase. Why waste _. uses_category_distribution to useDistribution? Do you like this?

    uses_category_distribution = models.BooleanField(default=False)
    points_for_correct_answer = models.FloatField(default=1)
    points_for_unanswered = models.FloatField(default=0)
    points_for_wrong_answer = models.FloatField(default=-0.5)
    minimum_passing_score = models.FloatField(default=5)

    def __str__(self):
        return self.name

It's good to see you are trying to document function but I believe that it helps a lot when we follow some standards. What do you think? Well this is a good resource: https://docs.python-guide.org/writing/documentation

    # returns 'amount' random questions that user hasn't seen yet; if category is specified,
    # the questions will be from that category
    # raises OutOfQuestionsException if there aren't enough questions that satisfy requirements

The function name could be more specific and suttle. Simply quesiton or makeQuestion or generateQuestion would be sufficient I believe!

    def get_questions_for(self, user, amount, category=None):

Would you think if we move this to another function it would help make this function be more clean? And probably just pass the ID here, which is easier to test? # get list of questions already seen by user seen_questions = SeenQuestion.objects.filter(user=user) seen_question_ids = map(lambda q: q.question.pk, seen_questions)

        questions = Question.objects.filter(course=self).exclude(
            id__in=seen_question_ids
        )

The code below has the option to choose a question if the category exits, else it's doing something. Im sorry but I was a bit confused. So instead of why don;t we try to find some better way to do it? This definitely looks like more conditions are required to clarify things. Also we can find some better way to use try/catch. if category: questions = questions.filter(category=category)

        # pick random questions
        try:
            random_questions = random.sample(list(questions), amount)
        except ValueError as err:  # if there aren't enough questions left, return None
            raise OutOfQuestionsException

        return random_questions


    # returns an object containing info about the course
    def get_aggregated_info(self):
        subscribers = apps.get_model(
            "users", model_name="CourseSpecificProfile"
        ).objects.filter(course=self)
        taken_tests = TakenTest.objects.filter(course=self)

        avg_score = (
            (sum([test.score for test in list(taken_tests)]) / taken_tests.count())
            if taken_tests.count() > 0
            else 0
        )

        return {
            "number_of_subscribers": subscribers.count(),
            "number_of_tests_taken": taken_tests.count(),
            "average_score": round(avg_score, 1),
        }


    # returns 'amount' questions, showing correct answer and solution too
    # (meant for use inside of course control panel)
    def get_complete_questions(self, amount, pk_greater_than=0, category=None):
        questions = self.question_set.filter(pk__gt=pk_greater_than)
        if category is not None:
            cat = Category.objects.get(pk=category)
            questions = questions.filter(category=cat)

        questions = questions.order_by("pk")[:amount]
        # TODO add sorting
        return list(
            map(
                lambda q: q.format_complete_question(),
                questions,
            )
        )


    # ? move this to CourseSpecificProfile
    def get_seen_questions(self, user, amount, pk_greater_than=0, category=None):
        questions = user.seenquestion_set.filter(pk__gt=pk_greater_than)

Condition are the important part of our understanding so IMHO it would be great to see a line above them. if category is not None: cat = Category.objects.get(pk=category) questions = questions.filter(category=cat)

        questions = questions.order_by("pk")[:amount]

Let's not have this comment here. Every implementation must be complete. # TODO add sorting

        return list(
            map(
                lambda q: q.serialize(),
                questions,
            )
        )


    # returns the 'quantity' hardest questions from this course,
    # i.e. those with the lowest percentage of times they were answered correctly
    def get_hardest_questions(self, quantity):
        return list(
            map(
                lambda q: q.format_complete_question(),
                self.question_set.all().order_by("percentage_of_correct_answers")[
                    :quantity
                ],
            )
        )

    # returns the last 'quantity' actions taken by course admins or collaborators
    def get_last_actions(self, quantity):
        return list(
            map(
                lambda a: a.serialize(),
                self.staffaction_set.all().order_by("-timestamp")[:quantity],
            )
        )

This functionality probably fits more in user model. What do you think? # returns the profiles of users who are subscribed to this course def get_subscribed_users(self): return list(map(lambda u: u.serialize(), self.coursespecificprofile_set.all()))

    # returns all the reports that have been made to questions from this course
    # if resolved is specified, only reports with that status are returned
    def get_reports(self, resolved=None):
        reports = Report.objects.filter(question__course=self)

        if resolved is not None:
            reports = reports.filter(resolved=resolved)
        return list(map(lambda r: r.serialize(), reports))

We can come up with a better function name :/) def maximum_score(self): return self.points_for_correct_answer * self.number_of_questions_per_test


From the model implementation perspective, I believe a Repository Pattern would be helpful. Furthermore, I can see a lot of lambda usage but it would be better if we can use write plain sql query to fetch the data instead of using objects instances of other model as that would be more efficient.


Recommended Read

This is really a great beginning in the start phases, a lot of learning awaits. I would recommend

Well this is a long, long, a long way to go but the effort is appreciable!

I wish you all the very best!

I hope it helps.

Cheers,

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.