Fix upload.py link when Django app is running on production (fixes issue 25) (issue4095042)

[tech...@gmail.com]

Reviewers: Andi Albrecht,

Please review this at http://codereview.appspot.com/4095042/

Affected files:
M examples/rietveld/README
A examples/rietveld/patches/download.link.diff
M examples/rietveld/settings.py

Index: examples/rietveld/patches/download.link.diff
===================================================================
--- examples/rietveld/patches/download.link.diff (revision 0)
+++ examples/rietveld/patches/download.link.diff (revision 0)
@@ -0,0 +1,13 @@
+Index: codereview/urls.py
+===================================================================
+--- codereview/urls.py (revision 649)
++++ codereview/urls.py (working copy)
+@@ -77,7 +77,7 @@
+ (r'^_ah/mail/(.*)', 'incoming_mail'),
+ (r'^xsrf_token$', 'xsrf_token'),
+ # patching upload.py on the fly
+- (r'^static/upload.py$', 'customized_upload_py'),
++ (r'^dynamic/upload.py$', 'customized_upload_py'),
+ (r'^search$', 'search'),
+ )
+
Index: examples/rietveld/settings.py
===================================================================
--- examples/rietveld/settings.py (revision 162)
+++ examples/rietveld/settings.py (working copy)
@@ -97,3 +97,5 @@

# This won't work with gae2django.
RIETVELD_INCOMING_MAIL_ADDRESS = None
+
+UPLOAD_PY_SOURCE = os.path.join(os.path.dirname(__file__), 'upload.py')
Index: examples/rietveld/README
===================================================================
--- examples/rietveld/README (revision 162)
+++ examples/rietveld/README (working copy)
@@ -66,7 +66,8 @@

patch -p0 < patches/upload.diff
patch -p0 < patches/account-login-links.diff
-
+ patch -p0 < download.link.diff
+
Finally run

./manage.py syncdb

[albrec...@googlemail.com]

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/README
File examples/rietveld/README (right):

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/README#newcode69
examples/rietveld/README:69: patch -p0 < download.link.diff
This should go into the Makefile too (sorry, the last one too, I've
missed that).

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/patches/download.link.diff
File examples/rietveld/patches/download.link.diff (right):

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/patches/download.link.diff#newcode10
examples/rietveld/patches/download.link.diff:10: +

(r'^dynamic/upload.py$', 'customized_upload_py'),

/static/upload.py is still linked from the HTML pages, e.g. when
clicking on "Create Issue".

http://codereview.appspot.com/4095042/

[tech...@gmail.com]

http://codereview.appspot.com/4095042/

[tech...@gmail.com]

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/README
File examples/rietveld/README (right):

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/README#newcode69
examples/rietveld/README:69: patch -p0 < download.link.diff

On 2011/01/22 07:27:33, Andi Albrecht wrote:
> This should go into the Makefile too (sorry, the last one too, I've
missed
> that).

Done.

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/patches/download.link.diff
File examples/rietveld/patches/download.link.diff (right):

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/patches/download.link.diff#newcode10
examples/rietveld/patches/download.link.diff:10: +
(r'^dynamic/upload.py$', 'customized_upload_py'),

On 2011/01/22 07:27:33, Andi Albrecht wrote:
> /static/upload.py is still linked from the HTML pages, e.g. when
clicking on
> "Create Issue".

I've updated Makefile with a newer revision of Rietveld. This should fix
the issue.

http://codereview.appspot.com/4095042/

[albrec...@googlemail.com]

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/patches/download.link.diff
File examples/rietveld/patches/download.link.diff (right):

http://codereview.appspot.com/4095042/diff/1/examples/rietveld/patches/download.link.diff#newcode10
examples/rietveld/patches/download.link.diff:10: +
(r'^dynamic/upload.py$', 'customized_upload_py'),

On 2011/01/22 09:05:16, techtonik wrote:
> On 2011/01/22 07:27:33, Andi Albrecht wrote:
> > /static/upload.py is still linked from the HTML pages, e.g. when
clicking on
> > "Create Issue".

> I've updated Makefile with a newer revision of Rietveld. This should
fix the
> issue.

hm, I don't see how updating to a newer version fixes this.

In newer versions of Rietveld "/static/upload.py" is replaced by "{{
media_url }}upload.py" which is contradictory to your change. As far as
I understand your change you don't want upload.py to be served from
{{media_url}}, isn't it?

http://codereview.appspot.com/4095042/diff/6001/examples/rietveld/Makefile
File examples/rietveld/Makefile (right):

http://codereview.appspot.com/4095042/diff/6001/examples/rietveld/Makefile#newcode1
examples/rietveld/Makefile:1: RIETVELDREV=650
Updating the revision isn't that easy. The drawback is that the patches
need to be updated too. When I apply this patch and run "make all" two
patches fail. Actually that's the reason why I don't use always the
trunk version - read "RIETVELDREV=xxx" as "known to be working with rev
xxx" :)

http://codereview.appspot.com/4095042/

[tech...@gmail.com]

On 2011/01/22 09:12:59, Andi Albrecht wrote:
> In newer versions of Rietveld "/static/upload.py" is replaced by "{{
media_url
> }}upload.py" which is contradictory to your change. As far as I
understand your
> change you don't want upload.py to be served from {{media_url}}, isn't
it?

Yes, but r644 changed {{media_url}} to {%url
codereview.views.customized_upload_py%}"> -
http://code.google.com/p/rietveld/source/browse/trunk/templates/new.html#11
;)

> examples/rietveld/Makefile:1: RIETVELDREV=650
> Updating the revision isn't that easy. The drawback is that the
patches need to
> be updated too. When I apply this patch and run "make all" two patches
fail.
> Actually that's the reason why I don't use always the trunk version -
read
> "RIETVELDREV=xxx" as "known to be working with rev xxx" :)

That's a pain. May these patches fail, because already patched? Or
because clear_rietveld should be run first?

http://codereview.appspot.com/4095042/

[Andi Albrecht]

On Sat, Jan 22, 2011 at 10:35 AM, <tech...@gmail.com> wrote:
> On 2011/01/22 09:12:59, Andi Albrecht wrote:
>>
>> In newer versions of Rietveld "/static/upload.py" is replaced by "{{
>
> media_url
>>
>> }}upload.py" which is contradictory to your change. As far as I
>
> understand your
>>
>> change you don't want upload.py to be served from {{media_url}}, isn't
>
> it?
>
> Yes, but r644 changed {{media_url}} to {%url
> codereview.views.customized_upload_py%}"> -
> http://code.google.com/p/rietveld/source/browse/trunk/templates/new.html#11
> ;)

sorry, my mistake. My working copy wasn't up to date on this machine.
You're right, of course :)

>
>> examples/rietveld/Makefile:1: RIETVELDREV=650
>> Updating the revision isn't that easy. The drawback is that the
>
> patches need to
>>
>> be updated too. When I apply this patch and run "make all" two patches
>
> fail.
>>
>> Actually that's the reason why I don't use always the trunk version -
>
> read
>>
>> "RIETVELDREV=xxx" as "known to be working with rev xxx" :)
>
> That's a pain. May these patches fail, because already patched? Or
> because clear_rietveld should be run first?

The patches fail because the don't apply properly anymore. And yes,
it's best to run clear_rietveld to force the whole procedure to start
over again.

>
>
> http://codereview.appspot.com/4095042/
>

[tech...@gmail.com]

On 2011/01/22 09:40:46, Andi Albrecht wrote:
> >
> >> examples/rietveld/Makefile:1: RIETVELDREV=650
> >> Updating the revision isn't that easy. The drawback is that the
> >
> > patches need to
> >>
> >> be updated too. When I apply this patch and run "make all" two
patches
> >
> > fail.
> >>
> >> Actually that's the reason why I don't use always the trunk version
-
> >
> > read
> >>
> >> "RIETVELDREV=xxx" as "known to be working with rev xxx" :)
> >
> > That's a pain. May these patches fail, because already patched? Or
> > because clear_rietveld should be run first?

> The patches fail because the don't apply properly anymore. And yes,
> it's best to run clear_rietveld to force the whole procedure to start
> over again.

That's strange. I've just applied them manually to Rietveld trunk
without any conflicts, but I work on Windows. Have you tried 'svn st
codereview/ templates/' to see if there are any conflicts unresolved
already in these files?

> >
> > http://codereview.appspot.com/4095042/
> >

http://codereview.appspot.com/4095042/

[Andi Albrecht]

On Sat, Jan 22, 2011 at 11:16 AM, <tech...@gmail.com> wrote:

This is what happens here (current SVN with your patch applied):

$ make all
svn export http://rietveld.googlecode.com/svn/trunk/upload.py@650
A upload.py
Export complete.
patch -p0 < patches/upload.diff
patching file upload.py
patch -p0 < patches/account-login-links.diff
patching file templates/issue_base.html
Hunk #1 FAILED at 28.
1 out of 1 hunk FAILED -- saving rejects to file templates/issue_base.html.rej
patching file templates/base.html
Hunk #1 FAILED at 168.
1 out of 1 hunk FAILED -- saving rejects to file templates/base.html.rej
make: *** [upload.py] Error 1

>
>> >
>> > http://codereview.appspot.com/4095042/
>> >
>
>
>
> http://codereview.appspot.com/4095042/
>

[tech...@gmail.com]

On 2011/01/22 10:22:40, Andi Albrecht wrote:
> This is what happens here (current SVN with your patch applied):

> $ make all
> svn export http://rietveld.googlecode.com/svn/trunk/upload.py%40650

> A upload.py
> Export complete.
> patch -p0 < patches/upload.diff
> patching file upload.py
> patch -p0 < patches/account-login-links.diff
> patching file templates/issue_base.html
> Hunk #1 FAILED at 28.
> 1 out of 1 hunk FAILED -- saving rejects to file
templates/issue_base.html.rej
> patching file templates/base.html
> Hunk #1 FAILED at 168.
> 1 out of 1 hunk FAILED -- saving rejects to file
templates/base.html.rej
> make: *** [upload.py] Error 1

I don't have any other advice than to run `make clean_rietveld all` and
check that files `svn st templates/` is clean. There is no other logical
explanation. Manual checkout and patch works ok. Just tested this again.

http://codereview.appspot.com/4095042/

[Andi Albrecht]

ok, I've got it. When running make all the patches are applied (during
"upload.py" target) *before* templates/ is checked out.

Do you have make installed so you can fix the build order? If not,
just commit it and I'll fix it later.

Andi

>
>
> http://codereview.appspot.com/4095042/
>

[tech...@gmail.com]

I've committed the patch to r163. Unfortunately, there is no `make` on
my Vista.

http://codereview.appspot.com/4095042/

[Andi Albrecht]

ok, "make all" works again (r164).

[tech...@gmail.com]

Nice! Thanks for review. =)

http://codereview.appspot.com/4095042/