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

Set login form to use WTForms #321

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

NogaOs
Copy link
Contributor

@NogaOs NogaOs commented Oct 1, 2021

No description provided.

@NogaOs NogaOs requested a review from yammesicka October 1, 2021 21:16
@NogaOs NogaOs self-assigned this Oct 1, 2021
@NogaOs NogaOs added the good first issue Good for newcomers label Oct 1, 2021
@sourcery-ai
Copy link

sourcery-ai bot commented Oct 1, 2021

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 0.02%.

Quality metrics Before After Change
Complexity 3.72 ⭐ 3.66 ⭐ -0.06 👍
Method Length 52.73 ⭐ 52.73 ⭐ 0.00
Working memory 6.99 🙂 7.00 🙂 0.01 👎
Quality 74.57% 🙂 74.59% 🙂 0.02% 👍
Other metrics Before After Change
Lines 612 616 4
Changed files Quality Before Quality After Quality Change
lms/lmsweb/views.py 74.55% 🙂 74.56% 🙂 0.01% 👍
lms/lmsweb/forms/reset_password.py 75.87% ⭐ 75.87% ⭐ 0.00%

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

File Function Complexity Length Working Memory Quality Recommendation
lms/lmsweb/views.py comment 13 🙂 202 😞 8 🙂 50.15% 🙂 Try splitting into smaller methods
lms/lmsweb/views.py view 8 ⭐ 140 😞 10 😞 56.16% 🙂 Try splitting into smaller methods. Extract out complex expressions
lms/lmsweb/views.py login 4 ⭐ 127 😞 12 😞 57.91% 🙂 Try splitting into smaller methods. Extract out complex expressions
lms/lmsweb/views.py note 6 ⭐ 144 😞 8 🙂 61.90% 🙂 Try splitting into smaller methods
lms/lmsweb/views.py shared_solution 2 ⭐ 84 🙂 10 😞 69.27% 🙂 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!

Copy link
Member

@yammesicka yammesicka left a comment

Choose a reason for hiding this comment

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

Great start! I've added few comments that can help improve this PR further :)

lms/lmsweb/forms/reset_password.py Show resolved Hide resolved
lms/lmsweb/views.py Show resolved Hide resolved
login_message = request.args.get('login_message')
if not form.validate_on_submit():
return render_template(
'login.html', form=form, login_message=login_message,
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the login message from the last page request or generate new one? (I might have missed something in the logic here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I changed anything meaningful here. Me missing something in the logic here would be more probable :P
From just messing around with the system, it seems to work - what should I fix here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to create a form validator instead of the auth method and because of that you would remove 113-122 lines.
You can take a look in the lmsweb/tools/validators.py and the auth method in order to create the validator

Comment on lines +5 to +7
<div class="container">
<div id="login-container">
<div id="login" class="text-center">
Copy link
Member

Choose a reason for hiding this comment

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

Please configure your IDE to convert TAB to 2 spaces in HTML

<hr class="mt-3 mb-3">
<a href="/" class="btn btn-success btn-sm" role="button">{{ _('Back to login page') }}</a>
</div>
<div class="container">
Copy link
Member

Choose a reason for hiding this comment

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

Please reindent it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants