login and logout redirection done

login and logout redirection done

[Thomas.J.Waldmann, 2015-01-22 20:17:22]

some feedback.

in general: which issue (issue tracker url?) are you fixing?

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (left):

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1574: 
same here: do not remove empty lines without reason.

in case you want to do whitespace-cleanup, this (and only this) should be in a
separate changeset, not mixed with real code changes.

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:74: 
you inserted an empty line here. if this was not to fix a pep8 issue, it will
introduce one.

so, please (if it wasn't a fix): don't do that.

run the tests after making a change and before submitting it to code review.
look at hg diff / hg status to see if you accidentally changed something.

do first review yourself to spot such minor issues and fix them.

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1582: session['loginredirect']=request.referrer;
please read and follow pep8 (the python style guide).

here: spaces around "=" (see line above), no semicolon at EOL.

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1592: return redirect(session['loginredirect'])
the key/value should not be left in the session forever, space there is precious
(it is a cookie).

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1594: return redirect(url_for('.show_root')) 
#if user is directly going to login page Keyerror will be raised then redirect
to home
avoid code duplication. rather first compute url for both cases and then
redirect to computed url.

also pep8 issue here: blank after "#"

explanation is either a bit unclear or wrong.

https://codereview.appspot.com/194330043/diff/1/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1609: return redirect(request.referrer)
if don't think a browser is required to send a referrer header. what happens if
it does not?

[Thomas.J.Waldmann, 2015-01-23 16:47:56]

https://codereview.appspot.com/194330043/diff/60001/MoinMoin/apps/frontend/vi...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/194330043/diff/60001/MoinMoin/apps/frontend/vi...
MoinMoin/apps/frontend/views.py:1584: session['next'] = request.args.get('next')
or '/'
.get() for dictionaries has 2 arguments, the 2nd optional one is the default
value.

also, "/" is not correct for any installation, you need url_for('.show_root')

https://codereview.appspot.com/194330043/diff/60001/MoinMoin/apps/frontend/vi...
MoinMoin/apps/frontend/views.py:1595: session.pop('next')
.pop() returns the value it pops, so you could just use that 2 lines above.

btw, a more elegant way than to first pass it via request data (query string)
and then via the session might be to keep it in the request data (either in the
query string or in the form data).

https://codereview.appspot.com/194330043/diff/60001/MoinMoin/apps/frontend/vi...
MoinMoin/apps/frontend/views.py:1611: next_url = request.args.get('next') or '/'
see above

[Thomas.J.Waldmann, 2015-02-08 16:10:57]

https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:1589: next_url = request.args.get('next',
default = url_for('.show_root') )
does it need that "default ="?

if so, follow pep8 and remove the blanks around "=".
if not, remove it completely.

https://codereview.appspot.com/194330043/diff/100001/MoinMoin/themes/basic/te...
File MoinMoin/themes/basic/templates/login.html (right):

https://codereview.appspot.com/194330043/diff/100001/MoinMoin/themes/basic/te...
MoinMoin/themes/basic/templates/login.html:11: {{ gen.form.open(form,
method="post", action=url_for('frontend.login', next = request.args.get('next',
default=url_for('.show_root') ))) }}
see other comment, except that you already removed the spaces here.

[aabs08, 2015-02-13 16:48:43]

https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:1589: next_url = request.args.get('next',
default = url_for('.show_root') )
On 2015/02/08 16:10:57, Thomas.J.Waldmann wrote:
> does it need that "default ="?
> 
> if so, follow pep8 and remove the blanks around "=".
> if not, remove it completely.

yes it does need default else for "http://localhost:8080/+login" it is
redirected to http://localhost:8080/None as next is not present

[Thomas.J.Waldmann, 2015-02-15 17:03:22]

On 2015/02/13 16:48:43, aabs08 wrote:
>
https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
> File MoinMoin/apps/frontend/views.py (right):
> 
>
https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
> MoinMoin/apps/frontend/views.py:1589: next_url = request.args.get('next',
> default = url_for('.show_root') )
> On 2015/02/08 16:10:57, Thomas.J.Waldmann wrote:
> > does it need that "default ="?
> > 
> > if so, follow pep8 and remove the blanks around "=".
> > if not, remove it completely.
> 
> yes it does need default else for "http://localhost:8080/+login" it is
> redirected to http://localhost:8080/None as next is not present

I don't think you understood the point of my comment.
I was only referring to the "default = " part. I didn't say you shall remove
url_for('.show_root').

A normal dict has .get(key, default) signature, so you can just say foo.get(key,
"something") instead of foo.get(key, default="something").
Check if that is also the case for request.args (and practically test if it
works).

[Thomas.J.Waldmann, 2015-02-15 17:03:28]

https://codereview.appspot.com/194330043/diff/120001/MoinMoin/themes/basic/te...
File MoinMoin/themes/basic/templates/layout.html (right):

https://codereview.appspot.com/194330043/diff/120001/MoinMoin/themes/basic/te...
MoinMoin/themes/basic/templates/layout.html:173: {% if login_url %}
hmm, login_url is checked here, but after your change, it is not used any more.

[Thomas.J.Waldmann, 2015-02-17 17:35:25]

On 2015/02/15 17:03:22, Thomas.J.Waldmann wrote:
> On 2015/02/13 16:48:43, aabs08 wrote:
> >
>
https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
> > File MoinMoin/apps/frontend/views.py (right):
> > 
> >
>
https://codereview.appspot.com/194330043/diff/100001/MoinMoin/apps/frontend/v...
> > MoinMoin/apps/frontend/views.py:1589: next_url = request.args.get('next',
> > default = url_for('.show_root') )
> > On 2015/02/08 16:10:57, Thomas.J.Waldmann wrote:
> > > does it need that "default ="?
> > > 
> > > if so, follow pep8 and remove the blanks around "=".
> > > if not, remove it completely.
> > 
> > yes it does need default else for "http://localhost:8080/+login" it is
> > redirected to http://localhost:8080/None as next is not present
> 
> I don't think you understood the point of my comment.
> I was only referring to the "default = " part. I didn't say you shall remove
> url_for('.show_root').
> 
> A normal dict has .get(key, default) signature, so you can just say
foo.get(key,
> "something") instead of foo.get(key, default="something").
> Check if that is also the case for request.args (and practically test if it
> works).

still TODO.

[Thomas.J.Waldmann, 2015-02-17 17:35:33]

https://codereview.appspot.com/194330043/diff/180001/MoinMoin/themes/basic/te...
File MoinMoin/themes/basic/templates/layout.html (left):

https://codereview.appspot.com/194330043/diff/180001/MoinMoin/themes/basic/te...
MoinMoin/themes/basic/templates/layout.html:17: {% set login_url =
theme_supp.login_url() %}
is theme_supp.login_url still used somewhere?
also check same for "logout_url" method, if there is one.

https://codereview.appspot.com/194330043/diff/180001/MoinMoin/themes/basic/te...
File MoinMoin/themes/basic/templates/layout.html (right):

https://codereview.appspot.com/194330043/diff/180001/MoinMoin/themes/basic/te...
MoinMoin/themes/basic/templates/layout.html:173: </li>
please also fix block indentation after removing the outer if/endif block.

[Thomas.J.Waldmann, 2015-02-21 21:02:47]

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:442: def login_url(self, redirection=True):
how about just having a next=None parameter here? Maybe you do not need the
if/else then if it suppresses None value key/value pairs (try it).

would be also a bit more flexible as the caller can tell what should be next and
not limited to returning to current url (or staying on login url).

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/basic/te...
File MoinMoin/themes/basic/templates/layout.html (right):

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/basic/te...
MoinMoin/themes/basic/templates/layout.html:168: {{
hyperlink(url_for('frontend.logout', logout_submit=1,  next=current_path), None,
True,
only one space. did you run the pep8 checker?

[aabs08, 2015-02-22 05:55:11]

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/basic/te...
File MoinMoin/themes/basic/templates/layout.html (right):

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/basic/te...
MoinMoin/themes/basic/templates/layout.html:168: {{
hyperlink(url_for('frontend.logout', logout_submit=1,  next=current_path), None,
True,
On 2015/02/21 21:02:47, Thomas.J.Waldmann wrote:
> only one space. did you run the pep8 checker?

I did run ./m tests but now I see that they only check in *.py files and running
pep8 layput.html gives error in all the lines. Is there a checker for html files
also that I am unaware of?

[Thomas.J.Waldmann, 2015-02-22 17:02:04]

ok, looks good.

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/basic/te...
File MoinMoin/themes/basic/templates/layout.html (right):

https://codereview.appspot.com/194330043/diff/240001/MoinMoin/themes/basic/te...
MoinMoin/themes/basic/templates/layout.html:168: {{
hyperlink(url_for('frontend.logout', logout_submit=1,  next=current_path), None,
True,
On 2015/02/22 05:55:10, aabs08 wrote:
> On 2015/02/21 21:02:47, Thomas.J.Waldmann wrote:
> > only one space. did you run the pep8 checker?
> 
> I did run ./m tests but now I see that they only check in *.py files and
running
> pep8 layput.html gives error in all the lines. Is there a checker for html
files
> also that I am unaware of?

hmm, i think rogerhaase at some time did some template checking / cleanup, but i
don't remeber precisely - ask him.