Simplify loading of view restrictions in the menu. #1558

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
6 participants
@adaptivelogic
Contributor

adaptivelogic commented Dec 12, 2012

This refactoring aims to reduce the complexity of the query run when loading view restrictions in the menu code, solving #1224. When running with CMS_PERMISSION = False, the loading should be omitted entirely, saving considerable time.
The other aim is to move most of the view-restriction related code to cms.utils.permissions, allowing for sharing of cache and ensuring that both has_view_restriction and get_visible_sites are changed in parallel. Further, parts of the code was rewritten to enhance readability.

@digi604

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Dec 18, 2012

Contributor

The problem with the assertNumQueries is that mysql innoDB has save point queries that get counted as well... will have a look at this again after the multi db testing is merged.

Contributor

digi604 commented Dec 18, 2012

The problem with the assertNumQueries is that mysql innoDB has save point queries that get counted as well... will have a look at this again after the multi db testing is merged.

@digi604

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Dec 20, 2012

Contributor

remerge please

Contributor

digi604 commented Dec 20, 2012

remerge please

adaptivelogic added some commits Dec 20, 2012

Merge remote branch 'upstream/develop' into refactor-viewperms
Conflicts:
	cms/models/pagemodel.py
	cms/tests/menu.py
	cms/tests/permmod.py
	cms/utils/permissions.py
@digi604

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Dec 20, 2012

Contributor

Ok it seams you need to skip the tests with assertNumQueries if it is not sqlite
There are other tests that have this. search for @skipif

Contributor

digi604 commented Dec 20, 2012

Ok it seams you need to skip the tests with assertNumQueries if it is not sqlite
There are other tests that have this. search for @skipif

@adaptivelogic adaptivelogic referenced this pull request Jan 10, 2013

Closed

Truncated menu #1572

- page = Page()
- page.pk = 1
- page.level = 0
- page.tree_id = 1

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Jan 17, 2013

Contributor

lots of tests deleted. Why? Sure it still works?

@digi604

digi604 Jan 17, 2013

Contributor

lots of tests deleted. Why? Sure it still works?

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Jan 17, 2013

Contributor

sorry just saw the tests at the botton

@digi604

digi604 Jan 17, 2013

Contributor

sorry just saw the tests at the botton

@digi604

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Jan 17, 2013

Contributor

what are the improvements in the number of queries used?

Contributor

digi604 commented Jan 17, 2013

what are the improvements in the number of queries used?

@adaptivelogic

This comment has been minimized.

Show comment Hide comment
@adaptivelogic

adaptivelogic Jan 18, 2013

Contributor

The number of queries is pretty much the same, except when CMS_PERMISSION is False (this can be fixed with a much smaller changeset, however). The goal was more to reduce the complexity of the query that retrieves permissions, as the present one is hideously complex when looking at a large menu tree. See issue #1224 why this is a problem, and additionally, complex queries take a lot longer to evaluate than a simple test for number of queries would indicate. The load_ancestors method is the keystone for this, but you may want more tests before including in the main codebase. Ideally, this method does nothing in most cases as all the ancestors are present in the original queryset.

The other goal was to collect disparate parts of the code and place them centrally in cms.utils.permissions. This was only partially achieved, and may have to wait until 3.0 to be fully implemented.

Contributor

adaptivelogic commented Jan 18, 2013

The number of queries is pretty much the same, except when CMS_PERMISSION is False (this can be fixed with a much smaller changeset, however). The goal was more to reduce the complexity of the query that retrieves permissions, as the present one is hideously complex when looking at a large menu tree. See issue #1224 why this is a problem, and additionally, complex queries take a lot longer to evaluate than a simple test for number of queries would indicate. The load_ancestors method is the keystone for this, but you may want more tests before including in the main codebase. Ideally, this method does nothing in most cases as all the ancestors are present in the original queryset.

The other goal was to collect disparate parts of the code and place them centrally in cms.utils.permissions. This was only partially achieved, and may have to wait until 3.0 to be fully implemented.

@adaptivelogic

This comment has been minimized.

Show comment Hide comment
@adaptivelogic

adaptivelogic Jan 18, 2013

Contributor

Please hold on this one. Writing unit tests to satisfy myself that it doesn't break in new and exciting ways.

Contributor

adaptivelogic commented Jan 18, 2013

Please hold on this one. Writing unit tests to satisfy myself that it doesn't break in new and exciting ways.

@digi604

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Mar 28, 2013

Contributor

If you want to see this in 2.4 better hurry up 😄 I for one would love to see it.

Contributor

digi604 commented Mar 28, 2013

If you want to see this in 2.4 better hurry up 😄 I for one would love to see it.

@yakky

This comment has been minimized.

Show comment Hide comment
@yakky

yakky Jul 9, 2013

Contributor

@adaptivelogic As I told you at Europython, feel free to contact me if you want this merged

Contributor

yakky commented Jul 9, 2013

@adaptivelogic As I told you at Europython, feel free to contact me if you want this merged

@adaptivelogic

This comment has been minimized.

Show comment Hide comment
@adaptivelogic

adaptivelogic Jul 9, 2013

Contributor

I need to re-run the tests and make sure things work. Very busy at the
moment; Thursday at the earliest.

-- Björn

On 9 July 2013 20:20, Iacopo Spalletti notifications@github.com wrote:

@adaptivelogic https://github.com/adaptivelogic As I told you at
Europython, feel free to contact me if you want this merged

Contributor

adaptivelogic commented Jul 9, 2013

I need to re-run the tests and make sure things work. Very busy at the
moment; Thursday at the earliest.

-- Björn

On 9 July 2013 20:20, Iacopo Spalletti notifications@github.com wrote:

@adaptivelogic https://github.com/adaptivelogic As I told you at
Europython, feel free to contact me if you want this merged

@digi604

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Feb 10, 2014

Contributor

@adaptivelogic will this PR get any looooove again?

image

Contributor

digi604 commented Feb 10, 2014

@adaptivelogic will this PR get any looooove again?

image

@digi604 digi604 added this to the Some Day milestone Feb 10, 2014

@yakky yakky self-assigned this Feb 10, 2014

@yakky

This comment has been minimized.

Show comment Hide comment
@yakky

yakky Feb 10, 2014

Contributor

Assigning to myself to help to get this merged. @adaptivelogic: still interested in working on this PR?

Contributor

yakky commented Feb 10, 2014

Assigning to myself to help to get this merged. @adaptivelogic: still interested in working on this PR?

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 21, 2014

Coverage Status

Changes Unknown when pulling b7a58b9 on adaptivelogic:refactor-viewperms into * on divio:develop*.

Coverage Status

Changes Unknown when pulling b7a58b9 on adaptivelogic:refactor-viewperms into * on divio:develop*.

@yakky yakky modified the milestones: 3.X, Some Day Mar 1, 2014

yakky added a commit to yakky/django-cms that referenced this pull request Jul 23, 2014

yakky added a commit to yakky/django-cms that referenced this pull request Jul 23, 2014

@digi604

This comment has been minimized.

Show comment Hide comment
@digi604

digi604 Aug 14, 2014

Contributor

@yakky what is that status on this?

Contributor

digi604 commented Aug 14, 2014

@yakky what is that status on this?

@yakky

This comment has been minimized.

Show comment Hide comment
@yakky

yakky Aug 14, 2014

Contributor

Code is mostly merged in but the tests aren't. I'm planning this for 3.1

Il 14 agosto 2014 16:28:33 GMT+01:00, Patrick Lauber notifications@github.com ha scritto:

@yakky what is that status on this?


Reply to this email directly or view it on GitHub:
#1558 (comment)

-- Inviato dal mio cellulare Android con K-9 Mail.

Contributor

yakky commented Aug 14, 2014

Code is mostly merged in but the tests aren't. I'm planning this for 3.1

Il 14 agosto 2014 16:28:33 GMT+01:00, Patrick Lauber notifications@github.com ha scritto:

@yakky what is that status on this?


Reply to this email directly or view it on GitHub:
#1558 (comment)

-- Inviato dal mio cellulare Android con K-9 Mail.

@mkoistinen

This comment has been minimized.

Show comment Hide comment
@mkoistinen

mkoistinen Jan 12, 2015

Contributor

Wow, this PR is nearly 2 years old now. Still looks useful, but I worry about its ability to be merged against any recent code-base.

@yakky, can we get an update from your side? Is this still pegged for 3.1?

Contributor

mkoistinen commented Jan 12, 2015

Wow, this PR is nearly 2 years old now. Still looks useful, but I worry about its ability to be merged against any recent code-base.

@yakky, can we get an update from your side? Is this still pegged for 3.1?

@yakky

This comment has been minimized.

Show comment Hide comment
@yakky

yakky Jan 12, 2015

Contributor

It was still really hard to merge this in 3.0, but I'll hard try to port it to 3.1.
It's not mergeable as-is but I hope to be able to port the code anyway

Contributor

yakky commented Jan 12, 2015

It was still really hard to merge this in 3.0, but I'll hard try to port it to 3.1.
It's not mergeable as-is but I hope to be able to port the code anyway

yakky added a commit to yakky/django-cms that referenced this pull request Feb 8, 2015

yakky added a commit to yakky/django-cms that referenced this pull request Feb 8, 2015

@yakky yakky referenced this pull request Feb 8, 2015

Merged

Merge #1558 #3834

yakky added a commit to yakky/django-cms that referenced this pull request Feb 23, 2015

yakky added a commit to yakky/django-cms that referenced this pull request Feb 23, 2015

yakky added a commit to yakky/django-cms that referenced this pull request Feb 24, 2015

yakky added a commit to yakky/django-cms that referenced this pull request Feb 24, 2015

@yakky yakky closed this in #3834 Mar 1, 2015

yakky added a commit that referenced this pull request Mar 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment