658966 - [accounts] AJAX login to use JSON instead and to load login form always

Status: VERIFIED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Peter Bengtsson [:peterbe]]

Problems I'd like to solve:

* On every page an AJAX GET updates the header (upper right hand corner) with your name if you're logged in. This sends a rather large chunk of HTML and it's only loaded *after* the whole DOM tree is loaded (jQuery.ready(document)) meaning that if the page is slow to render you get unnecessary flicker as your name appears quite late

* If not logged in, the login form is fetched via a AJAX GET meaning every page loaded (if not logged in) sends the login form by AJAX. Also, since the login form is not part of the "original HTML" the browser password saver fails to work even though it asks if you want to save your password. 

* Upon logging in successfully with AJAX POST it redirects to a page that returns the HTML containing the "Hi <your name>" part. I'd rather the AJAX POST returns a 200 OK (instead of 302 Found) because AJAX and redirects can sometimes be problematic in certain browser (at the time no external links to prove this)

Comment 1

[Peter Bengtsson [:peterbe]]

Note: I've made this depend on bug 658966 because that bug has a branch that re organizes the use of all external Javascript.

Comment 2

[Staś Małolepszy :stas]

(In reply to comment #1)
> Note: I've made this depend on bug 658966 [snip]

That should say bug 563236.

Comment 3

[Peter Bengtsson [:peterbe]]

Attachment: Patch to turn login into a 100% AJAX feature

Note: this patch is based on the branch for https://bugzilla.mozilla.org/show_bug.cgi?id=563236 (which blocks this bug anyway)

This patch solves the problem with the Password Manager because the login form is loaded on every page (bug hidden). 

The AJAX to check if you're logged in only fetches a tiny JSON struct instead of a big block of HTML which needs to be inserted into the DOM tree. The payload is now about 50 bytes whereas before it was 700+ bytes (for logged in AJAX requests)

An "ugly" thing about this patch is the way it wraps the 'django.contrib.auth.views.login' view. Because there is no other way to get form submission errors by JSON I had to do it this way. It's idiomatic in that we're still leveraging the core of that view function at least. 

All new python code has 100% test coverage.

Comment 4

[Axel Hecht [:Pike]]

Comment on attachment 539184 [details] [diff] [review]
Patch to turn login into a 100% AJAX feature

The changes to apps/tinder/tests.py look unrelated, are they?

Comment 5

[Peter Bengtsson [:peterbe]]

(In reply to comment #4)
> Comment on attachment 539184 [details] [diff] [review] [review]
> Patch to turn login into a 100% AJAX feature
> 
> The changes to apps/tinder/tests.py look unrelated, are they?

Indeed! Crap! I guess I accidentally worked in the wrong branch when juggling between them. 

That code in itself just looks like some cleaning up refactoring (turning self.assertTrue into ok_).

Shall I just remove that change from the patch?

Comment 6

[Axel Hecht [:Pike]]

Yes, please.

Comment 7

[Peter Bengtsson [:peterbe]]

(In reply to comment #6)
> Yes, please.

Bad news, because I removed the branch in the middle I can now not rebase this second fixup. 

Anyway, I've removed the changes to apps/tinder/tests.py but I can't create another diff patch because I can't rebase without the middle branch. 

I hope that once you r+ this I can at least do the feature finish.

Comment 8

[Axel Hecht [:Pike]]

That looks like it's fall out from a duplicate changesets of the refactor patch on your feature branch here. That shouldn't land.

I can see how that wouldn't rebase, but just creating a new feature branch and applying the patch should work wonders.

"Can't create a diff" is really a good indication for "shouldn't land".

Comment 9

[Peter Bengtsson [:peterbe]]

(In reply to comment #8)
> That looks like it's fall out from a duplicate changesets of the refactor
> patch on your feature branch here. That shouldn't land.
> 
> I can see how that wouldn't rebase, but just creating a new feature branch
> and applying the patch should work wonders.
> 
How do you mean? 
This is what I see (when I run `git flow feature rebase -i bug658966-ajax-login`):


pick b2cfb91 refactored several templates to use external .js files and ad-hoc CSS inside style.cs\
s instead with fixes for Pike's review, r=Pike
pick beb8f7a bug658966 - AJAX login, r=Pike
pick e2b92c2 revoking improved tests

Even without changing anything it starts to try to fix things and leaves me with a foobar'd state with merge conflicts in every file. 

Ideas for how to fix it welcome?

Comment 10

[Axel Hecht [:Pike]]

As I said, rebase is not going to work. Just take the diff, remove the tests stuff from the patch file, and 

git flow feature start bug658966-ajax-login2
git apply .../file.patch

and procede as usual. I'd kline the old feature/bug658966-ajax-login branch once things work on develop.

Comment 11

[Peter Bengtsson [:peterbe]]

Attachment: AJAX login with correct tinder tests

Turns out that all the changes made to apps/tinder/tests.py were relevant and needed except one or two lines where self.assertEqual was replaced with eq_

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 546516 [details] [diff] [review]
AJAX login with correct tinder tests

Review of attachment 546516 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 13

[Peter Bengtsson [:peterbe]]

Landed https://github.com/mozilla/elmo/commit/4901cc3b9e98c7fc72a80ed6cafbc8860ae731fe

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Post-fix screenshot

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on https://l10n-dev-sj.mozilla.org/.