Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add exercises tags #326

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 62 additions & 8 deletions lms/lmsdb/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,27 @@ def on_notification_saved(
instance.delete_instance()


class Tag(BaseModel):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now think about changing this tablename to ExerciseTag, and the connection to ExerciseToTag, in order to allow easy future expansion of the tags to user. IDK what the right approach to a state where we have both Users and Exercises that use tags, but I have a strong hunch that they shouldn't share the same table.

text = TextField()
course = ForeignKeyField(Course)
date_created = DateTimeField(default=datetime.now)

class Meta:
indexes = (
(('text', 'course_id'), True),
)

orronai marked this conversation as resolved.
Show resolved Hide resolved
@classmethod
def create_tag(cls, text: str, course: Course) -> 'Tag':
orronai marked this conversation as resolved.
Show resolved Hide resolved
instance, _ = cls.get_or_create(
**{cls.text.name: html.escape(text), cls.course.name: course},
)
return instance

def __str__(self):
return self.text


class Exercise(BaseModel):
subject = CharField()
date = DateTimeField()
Expand Down Expand Up @@ -376,12 +397,8 @@ def is_number_exists(cls, number: int) -> bool:
return cls.select().where(cls.number == number).exists()

@classmethod
def get_objects(
cls, user_id: int, fetch_archived: bool = False,
from_all_courses: bool = False,
):
user = User.get(User.id == user_id)
exercises = (
def by_user(cls, user_id: int):
return (
cls
.select()
.join(Course)
Expand All @@ -390,10 +407,20 @@ def get_objects(
.switch()
.order_by(UserCourse.date, Exercise.number, Exercise.order)
)

@classmethod
def get_objects(
cls, user_id: int, fetch_archived: bool = False,
from_all_courses: bool = False, exercise_tag: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should find a better way. Creating these godlike functions with trillion parameters is a bad habit. Can you please find another way to do it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really find a better way

):
user = User.get(User.id == user_id)
exercises = cls.by_user(user_id)
if not from_all_courses:
exercises = exercises.where(
UserCourse.course == user.last_course_viewed,
)
if exercise_tag:
exercises = Exercise.by_tags(exercises, exercise_tag)
if not fetch_archived:
exercises = exercises.where(cls.is_archived == False) # NOQA: E712
return exercises
Expand All @@ -408,12 +435,22 @@ def as_dict(self) -> Dict[str, Any]:
'exercise_number': self.number,
'course_id': self.course.id,
'course_name': self.course.name,
'tags': ExerciseTag.by_exercise(self),
}

@staticmethod
def as_dicts(exercises: Iterable['Exercise']) -> ExercisesDictById:
return {exercise.id: exercise.as_dict() for exercise in exercises}

@staticmethod
def by_tags(exercises: Iterable['Exercise'], exercise_tag: str):
return (
exercises
.join(ExerciseTag)
.join(Tag)
.where(Tag.text == exercise_tag)
)

def __str__(self):
return self.subject

Expand All @@ -426,6 +463,23 @@ def exercise_number_save_handler(model_class, instance, created):
instance.number = model_class.get_highest_number() + 1


class ExerciseTag(BaseModel):
exercise = ForeignKeyField(Exercise)
tag = ForeignKeyField(Tag)
date = DateTimeField(default=datetime.now)

@classmethod
def by_exercise(
cls, exercise: Exercise,
) -> Union[Iterable['ExerciseTag'], 'ExerciseTag']:
return (
cls
.select()
.where(cls.exercise == exercise)
.order_by(cls.date)
)


orronai marked this conversation as resolved.
Show resolved Hide resolved
class SolutionState(enum.Enum):
CREATED = 'Created'
IN_CHECKING = 'In checking'
Expand Down Expand Up @@ -561,11 +615,11 @@ def test_results(self) -> Iterable[dict]:
@classmethod
def of_user(
cls, user_id: int, with_archived: bool = False,
from_all_courses: bool = False,
from_all_courses: bool = False, exercise_tag: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same - we should try not to write such godlike functions

) -> Iterable[Dict[str, Any]]:
db_exercises = Exercise.get_objects(
user_id=user_id, fetch_archived=with_archived,
from_all_courses=from_all_courses,
from_all_courses=from_all_courses, exercise_tag=exercise_tag,
)
exercises = Exercise.as_dicts(db_exercises)
solutions = (
Expand Down
25 changes: 24 additions & 1 deletion lms/lmsweb/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,29 @@ def exercises_page():
)


@webapp.route('/exercises/<tag_name>')
orronai marked this conversation as resolved.
Show resolved Hide resolved
@login_required
def exercises_tag_page(tag_name: str):
fetch_archived = bool(request.args.get('archived'))
try:
solutions.check_tag_name(tag_name, current_user.last_course_viewed)
except LmsError as e:
error_message, status_code = e.args
return fail(status_code, error_message)

exercises = Solution.of_user(
current_user.id, fetch_archived, exercise_tag=tag_name,
)
is_manager = current_user.role.is_manager
return render_template(
'exercises.html',
exercises=exercises,
is_manager=is_manager,
fetch_archived=fetch_archived,
tag_name=tag_name,
)


@webapp.route('/notifications')
@login_required
def get_notifications():
Expand Down Expand Up @@ -479,7 +502,7 @@ def comment():

@webapp.route('/send/<int:course_id>/<int:_exercise_number>')
@login_required
def send(course_id: int, _exercise_number: Optional[int]):
def send(course_id: int, _exercise_number: Optional[int] = None):
if not UserCourse.is_user_registered(current_user.id, course_id):
return fail(403, "You aren't allowed to watch this page.")
return render_template('upload.html', course_id=course_id)
Expand Down
14 changes: 12 additions & 2 deletions lms/models/solutions.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
from playhouse.shortcuts import model_to_dict # type: ignore

from lms.extractors.base import File
from lms.lmsdb.models import SharedSolution, Solution, SolutionFile, User
from lms.lmsdb.models import (
Course, SharedSolution, Solution, SolutionFile, User,
)
from lms.lmstests.public.general import tasks as general_tasks
from lms.lmstests.public.identical_tests import tasks as identical_tests_tasks
from lms.lmsweb import config, routes
from lms.models import comments, notifications
from lms.models import comments, notifications, tags
from lms.models.errors import ForbiddenPermission, ResourceNotFound
from lms.utils.files import ALLOWED_IMAGES_EXTENSIONS

Expand Down Expand Up @@ -205,3 +207,11 @@ def get_files_tree(files: Iterable[SolutionFile]) -> List[Dict[str, Any]]:
for file in file_details:
del file['fullpath']
return file_details


def check_tag_name(tag_name: str, course: Course) -> None:
orronai marked this conversation as resolved.
Show resolved Hide resolved
if not tags.get_exercises_of(course, tag_name):
raise ResourceNotFound(
f'No such tag {tag_name} for course '
f'{current_user.last_course_viewed.name}.', 404,
)
33 changes: 33 additions & 0 deletions lms/models/tags.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
from typing import Iterable, Optional, Union

from lms.lmsdb.models import Course, Exercise, ExerciseTag, Tag


def get_exercises_of(
course: Course, tag_name: str,
orronai marked this conversation as resolved.
Show resolved Hide resolved
) -> Union[Iterable['ExerciseTag'], 'ExerciseTag']:
return (
ExerciseTag
.select(ExerciseTag.exercise)
.join(Tag)
.where(Tag.text == tag_name, Tag.course == course)
)


def of_exercise(
exercise_id: Optional[int] = None, course: Optional[int] = None,
number: Optional[int] = None,
orronai marked this conversation as resolved.
Show resolved Hide resolved
) -> Optional[Union[Iterable['ExerciseTag'], 'ExerciseTag']]:
if exercise_id is not None:
return ExerciseTag.select().where(ExerciseTag.exercise == id)
elif course is not None:
orronai marked this conversation as resolved.
Show resolved Hide resolved
tags = (
ExerciseTag
.select()
.join(Exercise)
.where(Exercise.course == course)
)
if number is not None:
tags = tags.where(Exercise.number == number)
return tags
return None
10 changes: 10 additions & 0 deletions lms/static/my.css
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,16 @@ a {
line-height: 3rem;
}

.exercise-tags {
align-items: center;
display: flex;
font-size: 1em;
height: 3rem;
justify-content: center;
text-align: center;
white-space: normal;
}

.exercise-send {
display: flex;
flex-direction: row;
Expand Down
7 changes: 6 additions & 1 deletion lms/templates/exercises.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<div id="exercises-page" class="page {{ direction }}">
<div id="exercises-header">
<div id="main-title">
<h1 id="exercises-head">{{ _('Exercises') }}</h1>
<h1 id="exercises-head">{{ _('Exercises') }}{% if tag_name %} - #{{ tag_name | e }}{% endif %}</h1>
yammesicka marked this conversation as resolved.
Show resolved Hide resolved
</div>
</div>
<div id="exercises">
Expand All @@ -14,6 +14,11 @@ <h1 id="exercises-head">{{ _('Exercises') }}</h1>
<div class="right-side {{ direction }}-language">
<div class="exercise-number me-3">{{ exercise['exercise_number'] }}</div>
<div class="exercise-name"><div class="ex-title">{{ exercise['exercise_name'] | e }}</div></div>
<div class="exercise-tags ms-1">
{% for tag in exercise.tags %}
<a class="ms-1" id="exercise-tag-link" href="{{ url_for('exercises_tag_page', tag_name=tag.tag) }}">#{{ tag.tag | e }}</a>
{% endfor %}
</div>
</div>
<div class="left-side">
<div class="comments-count">
Expand Down
2 changes: 1 addition & 1 deletion lms/templates/navbar.html
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
</a>
</li>
{% endif -%}
{%- if not exercises or fetch_archived %}
{%- if not exercises or fetch_archived or tag_name %}
<li class="nav-item">
<a href="/exercises" class="nav-link">
<i class="fa fa-book" aria-hidden="true"></i>
Expand Down
12 changes: 9 additions & 3 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
import pytest

from lms.lmsdb.models import (
ALL_MODELS, Comment, CommentText, Course, Exercise, Note, Notification,
Role, RoleOptions, SharedSolution, Solution, User, UserCourse,
ALL_MODELS, Comment, CommentText, Course, Exercise, ExerciseTag,
Tag, Note, Notification, Role, RoleOptions, SharedSolution,
Solution, User, UserCourse,
)
from lms.extractors.base import File
from lms.lmstests.public import celery_app as public_app
Expand Down Expand Up @@ -308,14 +309,19 @@ def create_exercise(
)


def create_exercise_tag(tag_text: str, course: Course, exercise: Exercise):
new_tag_id = Tag.create_tag(text=tag_text, course=course).id
return ExerciseTag.create(exercise=exercise, tag=new_tag_id)


def create_shared_solution(solution: Solution) -> SharedSolution:
return SharedSolution.create_new(solution=solution)


def create_note(
creator: User,
user: User,
note_text: CommentText,
note_text: str,
privacy: int,
):
new_note_id = CommentText.create_comment(text=note_text).id
Expand Down
36 changes: 36 additions & 0 deletions tests/test_exercises.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import datetime

from lms.lmsdb.models import Course, Exercise, User
from lms.models import tags
from tests import conftest


Expand Down Expand Up @@ -52,3 +53,38 @@ def test_courses_exercises(
template, _ = captured_templates[-1]
assert template.name == 'exercises.html'
assert len(list(Exercise.get_objects(student_user.id))) == 2

@staticmethod
def test_exercise_tags(
orronai marked this conversation as resolved.
Show resolved Hide resolved
student_user: User, course: Course, exercise: Exercise,
):
client = conftest.get_logged_user(username=student_user.username)
conftest.create_usercourse(student_user, course)
client.get(f'course/{course.id}')
conftest.create_exercise_tag('tag1', course, exercise)
tag_response = client.get('/exercises/tag1')
assert tag_response.status_code == 200

course2 = conftest.create_course(index=1)
exercise2 = conftest.create_exercise(course2, 2)
conftest.create_usercourse(student_user, course2)
conftest.create_exercise_tag('tag2', course2, exercise2)
bad_tag_response = client.get('/exercises/tag2')
assert bad_tag_response.status_code == 404

client.get(f'course/{course2.id}')
success_tag_response = client.get('/exercises/tag2')
assert success_tag_response.status_code == 200

another_bad_tag_response = client.get('/exercises/wrongtag')
assert another_bad_tag_response.status_code == 404

@staticmethod
def test_course_tags(course: Course, exercise: Exercise):
course2 = conftest.create_course(index=1)
conftest.create_exercise_tag('tag1', course, exercise)
conftest.create_exercise_tag('tag2', course, exercise)
exercise2 = conftest.create_exercise(course, 2)
conftest.create_exercise_tag('tag1', course, exercise2)
assert len(tags.of_exercise(course=course.id)) == 3
assert len(tags.of_exercise(course=course2.id)) == 0