#57 Pluggable app URLs

Allow apps to declare there URLs rather than registering them into the global urls.py

relates #53 A step towards #43

See CONTRIBUTING.md for more details on implementation.

Thanks Jocelyn.

It looks good, I just have a few comments:

• according to the doc, exported_urlpatterns is optional, but I don't see code that handles the case where there is no exported_urlpatterns (i.e. use the app name as URL prefix)
• I don't understand the addition in the VPN models (db_table = 'vpn_vpnconfiguration')

Once this is fixed and the code has been run-tested for Illyse (I will do it), you have my ACK for merging :)

Ah, I see the reason why you forced the db table:

ProgrammingError at /vpn/2
relation "coin_vpnconfiguration" does not exist

It looks like by using the new apps.py features, Django now thinks that the VPN code belongs to the Coin app (for database table, for the admin, etc). It reflects the "new" recommended organisation (apps are supposed to be outside of the project directory), but it breaks quite a lot of things.

I think the best solution would be to move the vpn app to the toplevel of the git repository (it would actually make more sense, since it's an optional app). I will give it a try.

I think the best solution would be to move the vpn app to the toplevel of the git repository (it would actually make more sense, since it's an optional app). I will give it a try.

Although I agree (VPN is an optional app) I consider it's not a blocker for this PR (ok if I let the db_table=... for now). ok ?

according to the doc, exported_urlpatterns is optional, but I don't see code that handles the case where there is no exported_urlpatterns (i.e. use the app name as URL prefix)

Then you missed it :) that's here : https://code.ffdn.org/jocelyn/coin/src/6bbfd712e80799d6c55ff4750894c2de94ca6a78/coin/apps.py#L13

(I promiss I didn't change it afterwards :-p)

@zorun Then I think it's ready for testing, do you agree ?

Although I agree (VPN is an optional app) I consider it's not a blocker for this PR (ok if I let the db_table=... for now). ok ?

Well actually, no: even with the explicit db_table, the change breaks the VPN application in more ways (no VPN shows up in the admin anymore, and the user view gives a 404 even though an object exists). I think django-polymorphic really doesn't like the change of application name.

I will try to move the vpn app before this week-end.

Then you missed it :) that's here : https://code.ffdn.org/jocelyn/coin/src/6bbfd712e80799d6c55ff4750894c2de94ca6a78/coin/apps.py#L13

(I promiss I didn't change it afterwards :-p)

Ah right, it's in the meta-class, sorry :)

Be careful btw, rstrip is probably not what you intended here: think of the argument as a list of characters to remove, not as a string. For instance:

"lollipop.py".rstrip(".py") 'lollipo'

It will remove any trailing "p" and "y" in the filename...

I have moved the VPN application, and adapted your branch to the change: the result is in the pluggable-urls branch. The VPN app works completely fine in this new branch!

I think you can either merge the new branch directly and fix the rstrip() issue afterwards, or fix the issue and rebase before merging. It's not really important either way, but this small issue should be fixed :)

Once it's merged, we can work on #55, and finally close #53...

Thanks for the good work!

I fixed the rstrip() issue (and a few others), and merged the branch in master in b05e274424, thanks Jocelyn :)

Thanks, I had forgotten that rstrip thing to be honest :x.