GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Set login form to use WTForms by NogaOs · Pull Request #321 · PythonFreeCourse/lms

Conversation

NogaOs commented Oct 1, 2021

Copy link

Copy Markdown

Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

noga-acumen added 4 commits

September 28, 2021 16:42


          added the login form, and tons of notes

464687d


          minor changes

5abe6cd


          final touch ups

a2c6a93


          Merge branch 'master' - lang change merge conflict

1fb834a

NogaOs requested a review from yammesicka

October 1, 2021 21:16

NogaOs self-assigned this

Oct 1, 2021


          flake8 fixes

b2e4a42

NogaOs added the good first issue Good for newcomers label

Oct 1, 2021


          summore flake8 fixes

5715c51

sourcery-ai Bot commented Oct 1, 2021

Copy link

Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

yammesicka requested changes

Oct 2, 2021

View reviewed changes

yammesicka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lms/lmsweb/forms/reset_password.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lms/lmsweb/views.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lms/lmsweb/views.py

		login_message = request.args.get('login_message')
		if not form.validate_on_submit():
		return render_template(
		'login.html', form=form, login_message=login_message,

yammesicka Oct 2, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

NogaOs Oct 3, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orronai Oct 3, 2021

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lms/templates/login.html

Comment on lines +5 to +7

		<div class="container">
		<div id="login-container">
		<div id="login" class="text-center">

yammesicka Oct 2, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lms/templates/signup.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">

yammesicka Oct 2, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please reindent it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Reviewers

orronai

orronai left review comments

yammesicka

yammesicka requested changes

Requested changes must be addressed to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

NogaOs

Labels

good first issue

Good for newcomers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.