unpleasant experience trying to bisect a change or regression

[Tim Graham]

Hi,

I wanted to share my experience trying to update to django-cms 3.4. When I launch my app, I get a NoReverseMatch exception related to URL reversing, so I tried to use git bisect to find the commit where the behavior changed in the django-cms. This wasn't so pleasant as there were a long string of commits where I couldn't run my app due to "ImportError cannot import name 'has_page_change_permission'" coming from django-cms. It seems that django-cms may be broken in its history for quite a few commits (maybe during the massive "Bumped version to 3.4.0rc1" merge commit?) which makes it quite difficult to pin exactly why my app is failing. I'm not sure if this is a common issue anyone else has seen or if it could have been avoided, but I just wanted to emphasize that it's really helpful if django-cms is working after each commit to help in a case like this. I'll admit that I don't really understand how git merging works but I find Django's cherry picking of commits to make backports to be very reliable in terms of making history bisectable. Anyway, just wanted to share in case some process improvement could come of it. I'll try to debug the actual problem with my app later.

There are only skipped commits left to test.
The first bad commit could be any of:
035de0310f54ed4b2ddacf9bd715c3ab4e9de072
046dc3e937b18a4f16526257320aa25a7a143754
1c92e02c8bfaba544663c91c6d7efa30e40e0de0
70a5c61500d7d3c7f09f7844bb64db4beeff7a7c
4989c294bc21dd725690983a0034300d5c1770e5
f96c80357c7152ef46045842ad0edb9b27dddae7
0136232ded0b97605d88df3da0f42a7fa13fcda5
bc1ae40a84592436ef56ac93a21e3f5320324174
59d303bb0b07beecbbe3f18a2498e382d6022430
8e2e049eaaf85b01a1e1949ce6dec6366fca577e
153b3d5625117769354918fb28ae73c48b417a4d
c6e7ccd9c1e8fab5bc257abb6d9e6b268fb93f9a
c7617f3150e3fddc37ed2345aa2d38b6f0956ee1
365e503f7e21e924cfae13bf7a0f8364b2e98d4c
db2b69ba800033515aa8cdf907895cb7d7fb19be
aabeb755ee6d711f403370f1b4149b3de2e985fc
355b7671b60a43b53f60ff3f23be57f460e920b2
4fd0d66ecdc44fa081b1d19e8550320a00b59b76
a4c0cb5d99bee250b6f0c6e33f50503a8248ef7c
d41b42316ebacb236ca116a4b2aa324291da673e
e686f74542b08757a8c996df034909d158573780
08c51d30a00727fae9c47ba650cb088e7646f47b
9b4af053b1f3d90e52d4c054b83682aef05c57d2
af21e0c352df6a07d98587c1e8a31f072a6a43e8
16e3bbecb7c9bf5d77c56e49832f473b177de6cf
5c09f1d731aa644fb091941654a37843ff9eab7a
b4ca4580aaa7531644ccc6661614d21417d3ee7b
We cannot bisect more!

[czpython]

Hello Tim,

Sorry for trouble.

Following Django practices, we've started to only do one commit per PR which means that we no longer have a change with commits that work and others that don't.

The big "Bumped version to 3.4.0rc1" commit contains release only changes like PO file changes from Transifex,  compilemessages, etc.. it's why it shows as 5k lines +.

Regarding the ImportError cannot import name 'has_page_change_permission' error, this is caused by our refactor of the permissions logic.

has_page_change_permission was not a public API and as such got removed without a deprecation period, you can see the list of functions that were removed in the release notes.

Most of the functions there were replaced by more specific functions in cms.utils.page_permissions, if you still rely on the CMS permissions system feel free to ping me to get more information

on the new API. Like the old one, it's still private but now it's very unlikely to change for a bit.

[Tim Graham]

Thanks, Paulo, and I'm sorry, this was my fault. My app was indeed using has_page_change_permission(). It wasn't a problem with django-cms. Another change in django-cms caused the exception in my app to change to something else, but ultimately I resolved my problems by removing usage of has_page_change_permission() and fixing usage of remove features (cms_app.py to cms_apps.py and cms_toolbar.py to cms_toolbars.py).

One possible improvement for django-cms could be the release notes regarding how to upgrade for the permissions changes: http://docs.django-cms.org/en/latest/upgrade/3.4.html#permissions

I've attached a diff of the change I made to my app and maybe you can tell me if it's correct. In particular, I couldn't tell the reason for both the Page.has_change_permission() method and the cms.utils.page_permissions.user_can_change_page() function. Since the former calls the latter, I can't see why the latter is needed.

page.has_change_permission(self.request.user)

seems simpler to me than (equivalent I think):

page_permissions.user_can_change_page(request.user, page=page)

Thanks!

[czpython]

Thanks for suggestion.

The permissions API is a bit tricky, we want to avoid exposing it too much until we know it's ready to be used as a "public" API. That said, I do think we can improve the release notes.

What do you think about us adding a note or warning in the release notes that targets any user who has a custom page extension and points them to the updated docs for it? While looking into this, I've noticed the page extension docs have not been updated, thanks for that :)

The diff you posted looks good, the only thing I would change is to use page_permissions.user_can_change_page() directly.

We decided to keep Page.has_change_permission() and the other Page.has_x_permission() methods for backwards compatibility but the plan is to deprecate them.

Our reasoning for enforcing the usage of page_permissions.user_can_x() functions is that these are already abstracting a lot of logic and as such, the Page.has_x_permission() methods seem like an extra unnecessary abstraction.

Also we wanted to keep all permission related logic in one place (vs in utility and models) and make these operations as consistent as possible in they way they're used, some permission checks like change and view are bound to a page instance but others like adding a root page or reverting a page are not.

So while I agree that page.has_change_permission() is simpler than calling it's page_permissions counterpart, there are cases where this gets confusing like:

page.has_add_permission() is not the same as page_permissions.user_can_add_page(), the latter is for adding a root page while the former is for adding a subpage.

[Tim Graham]

Yes, I probably copied that example from the documentation, so it's not unreasonable that others have similar code they'll need to update. A link would certainly help. I'll update my code accordingly, thanks.

[Daniele Procida]

On Thu, Sep 1, 2016, Tim Graham <timog...@gmail.com> wrote:

>I'll
>admit that I don't really understand how git merging works but I find
>Django's cherry picking of commits to make backports to be very reliable in
>terms of making history bisectable.

Hi Tim.

We're still trying to work out the right way to do this.

See <https://groups.google.com/forum/#!topic/django-cms-developers/0j20E9_5gOA>.

As I understand, when a commit is cherry-picked into a different branch, it acquires a different hash, so there is then no easy way to work out whether the same code is in both branches (they don't share a commit history that can be compared).

This makes it difficult to ensure that things are correctly ported forward or backwards when required.

On the other hand, merging (which does preserve hashes/history) is an all-or-nothing process, so not always suitable.

I'd love to know a way out of this dilemma.

Daniele

[Tim Graham]

Correct, we never use git merge in Django. We always cherry-pick (backport) at the time of making the initial commit, but since we commit everything to master rather than a stable branch, our situation is a bit different.

[Tim Graham]

Just wanted to follow up on a positive note and say "nice work on the new release". It was an easy upgrade after I fixed my own issues.

[czpython]

Thanks Tim :)