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
Changes from all 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
@@ -375,6 +375,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),
)

@classmethod
def create_tag(cls, course: Course, text: str) -> 'Tag':
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()
@@ -416,12 +437,8 @@ def is_number_exists(cls, course: Course, number: int) -> bool:
)

@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)
@@ -430,10 +447,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
@@ -448,12 +475,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

@@ -465,6 +502,23 @@ def exercise_number_save_handler(model_class, instance, created):
instance.number = model_class.get_highest_number(instance.course) + 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)
)


class SolutionState(enum.Enum):
CREATED = 'Created'
IN_CHECKING = 'In checking'
@@ -638,11 +692,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 = (
27 changes: 26 additions & 1 deletion lms/lmsweb/views.py
Original file line number Diff line number Diff line change
@@ -357,6 +357,31 @@ def exercises_page():
)


@webapp.route('/exercises/<tagname>')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe /exercises/tag/<tagname>? Or /tags/exercise/_ in case we'll want to tag other things in the future (users, maybe?)

@login_required
def exercises_tag_page(tagname: str):
fetch_archived = bool(request.args.get('archived'))
try:
solutions.is_tag_name_exists(
tagname, current_user.last_course_viewed.id,
)
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=tagname,
)
is_manager = current_user.role.is_manager
return render_template(
'exercises.html',
exercises=exercises,
is_manager=is_manager,
fetch_archived=fetch_archived,
tag_name=tagname,
)


@webapp.route('/notifications')
@login_required
def get_notifications():
@@ -480,7 +505,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)
10 changes: 9 additions & 1 deletion lms/models/solutions.py
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@
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

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


def is_tag_name_exists(tag_name: str, course_id: int) -> None:
if not tags.get_exercises_of(course_id, tag_name):
raise ResourceNotFound(
f'No such tag {tag_name} for course '
f'{current_user.last_course_viewed.name}.', 404,
)
30 changes: 30 additions & 0 deletions lms/models/tags.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from typing import Iterable, Union

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


def get_exercises_of(
course_id: int, tag_name: str,
) -> Union[Iterable['ExerciseTag'], 'ExerciseTag']:
return (
ExerciseTag
.select(ExerciseTag.exercise)
.join(Tag)
.where(Tag.text == tag_name, Tag.course == course_id)
)


def by_exercise_id(
exercise_id: int,
) -> Union[Iterable['ExerciseTag'], 'ExerciseTag']:
return ExerciseTag.select().where(ExerciseTag.exercise == exercise_id)


def by_course(course: int) -> Union[Iterable['ExerciseTag'], 'ExerciseTag']:
return ExerciseTag.select().join(Exercise).where(Exercise.course == course)


def by_exercise_number(
course: int, number: int,
) -> Union[Iterable['ExerciseTag'], 'ExerciseTag']:
return by_course(course).where(Exercise.number == number)
10 changes: 10 additions & 0 deletions lms/static/my.css
Original file line number Diff line number Diff line change
@@ -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;
7 changes: 6 additions & 1 deletion lms/templates/exercises.html
Original file line number Diff line number Diff line change
@@ -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>
</div>
</div>
<div id="exercises">
@@ -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', tagname=tag.tag) }}">#{{ tag.tag | e }}</a>
{% endfor %}
</div>
</div>
<div class="left-side">
<div class="comments-count">
2 changes: 1 addition & 1 deletion lms/templates/navbar.html
Original file line number Diff line number Diff line change
@@ -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>
13 changes: 9 additions & 4 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -15,9 +15,9 @@
import pytest

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


def create_exercise_tag(tag_text: str, course: Course, exercise: Exercise):
new_tag_id = Tag.create_tag(course=course, text=tag_text).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
40 changes: 40 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


@@ -52,3 +53,42 @@ 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(
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.by_course(course=course.id)) == 3
assert len(
tags.by_exercise_number(course=course.id, number=exercise.number),
) == 2
assert len(tags.by_exercise_id(exercise_id=exercise2.id)) == 1
assert len(tags.by_course(course=course2.id)) == 0