Skip to content

feat: Add exercises tags - #326

Open
orronai wants to merge 7 commits into
masterfrom
exercise-tags
Open

feat: Add exercises tags#326
orronai wants to merge 7 commits into
masterfrom
exercise-tags

Conversation

@orronai

@orronaiorronai commented Oct 5, 2021

Copy link
Copy Markdown
Collaborator
  • Added tags per course
  • Added for the exercises.html template the tags
  • Added the tables of the tags
  • Added a test

@orronai
feat: Add exercises tags
1204de3
- Added tags per course
- Added for the exercises.html template the tags
- Added the tables of the tags
- Added a test
@orronaiorronai linked an issue Oct 5, 2021 that may be closed by this pull request
Tag Exercise #124
Open
@codecov

codecovBot commented Oct 5, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #326 (07c743c) into master (f3ed156) will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #326 +/- ##
==========================================
+ Coverage 84.08% 84.36% +0.28% 
==========================================
Files 63 64 +1 Lines 2903 2955 +52 ==========================================
+ Hits 2441 2493 +52 
Misses 462 462
Impacted FilesCoverage Δ
lms/lmsdb/models.py91.66% <100.00%> (+0.39%)⬆️
lms/lmsweb/views.py93.19% <100.00%> (+0.19%)⬆️
lms/models/solutions.py98.97% <100.00%> (+0.03%)⬆️
lms/models/tags.py100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3ed156...07c743c. Read the comment docs.

yammesicka
yammesicka requested changes Oct 6, 2021
View reviewed changes
Comment threadlms/templates/exercises.html Outdated
Comment threadlms/templates/exercises.html Outdated
Comment threadlms/models/solutions.py Outdated
Comment threadtests/test_exercises.py
Comment threadlms/lmsdb/models.py Outdated
Comment threadlms/lmsdb/models.py Outdated
Comment threadlms/lmsdb/models.py Outdated
Comment threadlms/lmsdb/models.py
Comment threadlms/lmsweb/views.py Outdated
Comment threadlms/lmsdb/models.py
@orronai
- Changed tags color
23d0479
- Added a bridge of tags in models folder
- Added course and date_created column to the tags table
- Added a constraint to the tags table
@orronai
Removed unused method
4c74eb2
yammesicka
yammesicka reviewed Oct 8, 2021
View reviewed changes
Comment threadlms/lmsdb/models.py Outdated
yammesicka
yammesicka requested changes Oct 8, 2021
View reviewed changes
Comment threadlms/lmsdb/models.py
@classmethod
def get_objects(
cls, user_id: int, fetch_archived: bool = False,
from_all_courses: bool = False, exercise_tag: Optional[str] = None,

@yammesickayammesickaOct 8, 2021

Copy link
Copy Markdown
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?

@orronaiorronaiOct 15, 2021

Copy link
Copy Markdown
CollaboratorAuthor

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

Comment threadlms/lmsdb/models.py
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,

@yammesickayammesickaOct 8, 2021

Copy link
Copy Markdown
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

Comment threadlms/lmsweb/views.py Outdated
Comment threadlms/models/solutions.py Outdated
Comment threadlms/models/tags.py Outdated
Comment threadlms/models/tags.py Outdated
Comment threadlms/models/tags.py Outdated
Comment threadlms/templates/exercises.html
@orronai
- Merged master
53fb793
- Splitted a function into 3 functions
- Changed some functions names
@orronai
Changed into on the /exercises route
200be26
@orronai
Fixed route attribute
07c743c
@sourcery-ai

sourcery-aiBot commented Oct 8, 2021

Copy link
Copy Markdown

Sourcery Code Quality Report

❌ Merging this PR will decrease code quality in the affected files by 0.07%.

Quality metricsBeforeAfterChange
Complexity2.05 ⭐1.95 ⭐-0.10 👍
Method Length35.66 ⭐36.04 ⭐0.38 👎
Working memory6.73 🙂6.73 🙂0.00
Quality80.72% ⭐80.65% ⭐-0.07% 👎
Other metricsBeforeAfterChange
Lines20352137102
Changed filesQuality BeforeQuality AfterQuality Change
lms/lmsdb/models.py84.22% ⭐84.45% ⭐0.23% 👍
lms/lmsweb/views.py74.68% 🙂74.84% 🙂0.16% 👍
lms/models/solutions.py74.06% 🙂74.41% 🙂0.35% 👍
tests/conftest.py90.98% ⭐90.89% ⭐-0.09% 👎
tests/test_exercises.py70.00% 🙂71.34% 🙂1.34% 👍

Here are some functions in these files that still need a tune-up:

FileFunctionComplexityLengthWorking MemoryQualityRecommendation
lms/lmsweb/views.pycomment13 🙂202 😞8 🙂50.15% 🙂Try splitting into smaller methods
lms/models/solutions.pyget_view_parameters5 ⭐139 😞12 😞55.53% 🙂Try splitting into smaller methods. Extract out complex expressions
lms/lmsweb/views.pyview8 ⭐140 😞10 😞56.16% 🙂Try splitting into smaller methods. Extract out complex expressions
lms/lmsweb/views.pylogin5 ⭐129 😞12 😞56.69% 🙂Try splitting into smaller methods. Extract out complex expressions
lms/lmsdb/models.pySolution._base_next_unchecked0 ⭐95 🙂20 ⛔57.42% 🙂Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.

Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

yammesicka
yammesicka requested changes Oct 30, 2021
View reviewed changes
Comment threadlms/lmsweb/views.py
)


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

@yammesickayammesickaOct 30, 2021

Copy link
Copy Markdown
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?)

Comment threadlms/lmsdb/models.py
instance.delete_instance()


class Tag(BaseModel):

@yammesickayammesickaOct 30, 2021

Copy link
Copy Markdown
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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yammesickayammesickayammesicka requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tag Exercise

2 participants

@orronai@yammesicka