Add support for Rietveld instance that permanently moved by support HTTP 301.

Add support for Rietveld instance that permanently moved by support HTTP 301.

Before this change, HTTP 301 would result in a permement error. Update upload.py
to accept HTTP 301 and uses the right host to get a cookie against. It results
in a slower operation due to the additional requests but makes a much smoother
transition while updating the links to the new rietveld instance.

Tested manually against an instance that returns HTTP 301 on all request to
a rietveld instance in a different domain.

BUG=248
TEST=Tested with a real instance being moved

[M-A, 2010-11-19 19:34:37]

Please take a look. It is important for a rietveld instance move that I want to
do this weekend. Note that I'll commit in our upload.py copy right away but I
prefer to not fork from upstream.

[M-A, 2010-12-01 21:51:56]

Ping

[Ilia Mirkin, 2010-12-05 16:45:58]

http://codereview.appspot.com/3227041/diff/9001/upload.py
File upload.py (right):

http://codereview.appspot.com/3227041/diff/9001/upload.py#newcode408
upload.py:408: self._Authenticate('%s://%s' % (url_loc[0], url_loc[1]))
This will conflict with the upload script required for a gae2django'd upload.py
(which adds another argument to authenticate). While that's certainly no reason
to prevent API changes, if they can be avoided, perhaps worth trying?

Could you just update self.host instead (and add a log line about it too,
perhaps?)

For reference:
http://code.google.com/p/django-gae2django/source/browse/trunk/examples/rietv...

[M-A, 2010-12-05 18:35:12]

http://codereview.appspot.com/3227041/diff/9001/upload.py
File upload.py (right):

http://codereview.appspot.com/3227041/diff/9001/upload.py#newcode408
upload.py:408: self._Authenticate('%s://%s' % (url_loc[0], url_loc[1]))
On 2010/12/05 16:45:58, Ilia Mirkin wrote:
> This will conflict with the upload script required for a gae2django'd
upload.py
> (which adds another argument to authenticate). While that's certainly no
reason
> to prevent API changes, if they can be avoided, perhaps worth trying?
> 
> Could you just update self.host instead (and add a log line about it too,
> perhaps?)

That would make sense. I can look at that.


> For reference:
>
http://code.google.com/p/django-gae2django/source/browse/trunk/examples/rietv...

Oh, I wonder why it's not simply integrated? My take would be to:
Rename _Authenticate to _AuthenticateGoogleAccount(), add the new function in
the diff as _AuthenticateDjango() and have _Authenticate() to simply call the
right function depending if --django is provided, or whatever your preference.

[Andi Albrecht, 2010-12-05 20:45:58]

On Sun, Dec 5, 2010 at 7:35 PM,  <maruel@chromium.org> wrote:
>
> http://codereview.appspot.com/3227041/diff/9001/upload.py
> File upload.py (right):
>
> http://codereview.appspot.com/3227041/diff/9001/upload.py#newcode408
> upload.py:408: self._Authenticate('%s://%s' % (url_loc[0], url_loc[1]))
> On 2010/12/05 16:45:58, Ilia Mirkin wrote:
>>
>> This will conflict with the upload script required for a gae2django'd
>
> upload.py
>>
>> (which adds another argument to authenticate). While that's certainly
>
> no reason
>>
>> to prevent API changes, if they can be avoided, perhaps worth trying?
>
>> Could you just update self.host instead (and add a log line about it
>
> too,
>>
>> perhaps?)
>
> That would make sense. I can look at that.
>
>
>> For reference:
>
>
http://code.google.com/p/django-gae2django/source/browse/trunk/examples/rietv...
>
> Oh, I wonder why it's not simply integrated? My take would be to:

Rietveld shouldn't depend on gae2django, it's a separate project and
I'm really hesitating to introduce any dependencies. The reason for
that is that gae2django shouldn't affect Rietveld's development in any
way. Even if a flag to choose authentication methods is almost a
trivial change it would introduce such a dependency. The drawback is
that gae2django won't be able to support all features that App Engine
supports (think of task queues, incoming mail, channels, ...) and I'd
love to keep a discussion regarding portability of features out of
Rietveld. However, it's easy to patch somthing like this in on the
gae2django side.

> Rename _Authenticate to _AuthenticateGoogleAccount(), add the new
> function in the diff as _AuthenticateDjango() and have _Authenticate()
> to simply call the right function depending if --django is provided, or
> whatever your preference.
>
> http://codereview.appspot.com/3227041/
>

[M-A, 2010-12-07 12:08:29]

Reduced the patch to 5 lines, I tested with double redirect on different
authentication domains. It's a bit odd because GetUserCredentials() doesn't use
the updated self.host so when there's 2 consecutive prompts, the user has no
idea of why and against which server it's being asked a credential but that's a
fairly specific use case that shouldn't happen in general.

[Andi Albrecht, 2010-12-10 14:29:31]

Looks good to me.

Would it be a good idea to print out a notice to the user when we encounter a
301?

[Andi Albrecht, 2010-12-17 15:39:10]

Committed r632 (finally, and sorry for the delay).

Thanks!