Closes #93 - adding in more flexibility to the pages app #96

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Member

rossbruniges commented Mar 4, 2013

  • allows nested pages

Warning - not built for super complex sites, and BEWARE setting infinite parents!!

@andrewhayward andrewhayward and 1 other commented on an outdated diff Mar 4, 2013

make_mozilla/pages/templates/pages/page.html
@@ -2,6 +2,7 @@
{% block page_title %}{{ page.title }} | {{ super() }}{% endblock %}
{% block page_id %}page-{{ page.path }}{% endblock %}
@andrewhayward

andrewhayward Mar 4, 2013

Member

One thing I noticed locally regarding this logic is that the main site navigation is tied to these pages IDs. That is, for example, the 'About' page tab is tied to #page-about. Currently that's fine - there can only ever be one page with an 'about' slug. However, with this new functionality, you could have multiple pages with the same slug, under different parents. It may not be an issue, but it's something to be aware of.

@rossbruniges

rossbruniges Mar 4, 2013

Member

My thinking was that we use the ID for the root page and for all the inner pages we use the section-class.

I think that should work out...

@andrewhayward

andrewhayward Mar 4, 2013

Member

That's the theory, yes. But the CSS still uses #page-about #header .about a. So if you create an 'about' page for mentors (for example), the 'about' menu item will be selected.

Also, it might be worth adding a (pseudo-)property/method to the page model (something like root) which would return the highest-level page in the tree. This way, section-X would be consistent even when you have nesting more than one deep. Right now, /a/b/c/ would be in a different section to /a/b/. Of course, that might be the idea, but I'd have thought both being in section-a would make more sense.

@rossbruniges

rossbruniges Mar 4, 2013

Member

real_path to the rescue (I think this should work...)

@andrewhayward andrewhayward commented on the diff Mar 4, 2013

make_mozilla/pages/models.py
show_subnav = models.BooleanField(default=False,
verbose_name='Show sub-navigation menu')
subnav_title = models.CharField(max_length=100, blank=True, null=True,
verbose_name='Menu title', help_text='This can be left blank if you do not need a title')
additional_content = models.TextField(blank=True, null=True)
+ def has_ancestor(self, page):
+ if not self.parent:
+ return False
+ if self.parent.id == page.id:
+ return True
+ return self.parent.has_ancestor(page)
+
+ def save(self, update_children=False, *args, **kwargs):
+ super(Page, self).save(*args, **kwargs)
+ # Recursively update the full path of any children
+ # when asked explicitly.
+ if update_children:
+ # Update path from parent if any.
+ target = self.parent if self.parent else self
@andrewhayward

andrewhayward Mar 4, 2013

Member

I'm not sure what's going on here. Why, if the option is "update_children", is self potentially self-updating here? I don't see how this is tied to whether self has a parent or not.

If passing self's parent through is required to update self's real path, then that shouldn't rely on update_children in order to work.

😕

@alfredo

alfredo Mar 4, 2013

Member

I think that the name of the parameter passed to the save method might be not the best one, probably better to rename it, since it is called exactly the same as the method, didn't think about it.

Because if real_path must take into consideration every segment of the tree to update itself and the children i was taking the parent into consideration.

I think @andrewhayward is right, update children shouldn't receive the parent as an argument, it should get the real_path of the parent and pass it on since that is the important part for updating the children if any.

parent_prefix = self.parent.real_path if self.parent else None
utils.update_children(self, prefix=parent_prefix)

Code not tested in the project.

@andrewhayward andrewhayward commented on the diff Mar 4, 2013

make_mozilla/pages/utils.py
@@ -0,0 +1,42 @@
+def update_children(page, prefix=None, walked_pages=None):
+ """Update the ``real_path`` of the descendants of the given ``Page``."""
+ walked_pages = walked_pages if walked_pages else []
+ # Perfix used to determine the children path.
@andrewhayward

andrewhayward Mar 4, 2013

Member

Minor nit, but that should probably read # Prefix used...

@andrewhayward andrewhayward commented on the diff Mar 4, 2013

make_mozilla/pages/utils.py
+ if prefix:
+ page_segment = prefix + '/' + page_segment
+ page.real_path = page_segment
+ page.save()
+ for child in page.children.all():
+ # Update the real path for this descendant.
+ if child in walked_pages:
+ # This page has been processed before, ignore.
+ continue
+ walked_pages.append(child)
+ update_children(child, page_segment, walked_pages)
+ return True
+
+
+def get_page_root(page, walked_pages=None):
+ """Dtermines the root of the given ``page`` if any."""
@andrewhayward

andrewhayward Mar 4, 2013

Member

'Determines the root...' // nit-picking

@andrewhayward andrewhayward commented on the diff Mar 4, 2013

make_mozilla/pages/admin.py
+ return
+ if parent == self.instance:
+ raise forms.ValidationError("Page can't be its own parent")
+ # The parent can't be part of the same tree.
+ # Retrieve all the descendants from the root of this instance.
+ # If any of the ancestor of the parent are part of this tree
+ # the parent will show up when walking the root of he instance tree.
+ try:
+ root = utils.get_page_root(self.instance)
+ except ValueError, e:
+ # An existing circular dependency detected in
+ # the instance structure.
+ raise forms.ValidationError(e)
+ descendants = utils.get_page_descendants(root)
+ if parent in descendants:
+ raise forms.ValidationError("Pae is already page of "
@andrewhayward

andrewhayward Mar 4, 2013

Member

Page is already part of... ?

@alfredo

alfredo Mar 4, 2013

Member

Continues on the next line to keep it PEP8. Strings in python can be concatenated this way.

edit: spelling.

@andrewhayward

andrewhayward Mar 4, 2013

Member

Sorry, that wasn't a Python problem, but rather an English problem. Sentence doesn't make sense :) Wrapping it in quotes, as per the original code, might have made a bit more sense.

raise forms.ValidationError("Page is already part of "?

@alfredo

alfredo Mar 4, 2013

Member

Hah, alright, gotcha! :D, Oops clumsy fingers...

@andrewhayward andrewhayward commented on the diff Mar 4, 2013

make_mozilla/pages/admin.py
+ self.fields['parent'].choices = choices
+
+ def clean_parent(self):
+ """Validate that the ``parent`` is not:
+ - Its own parent.
+ - Part of the existing tree.
+ """
+ parent = self.cleaned_data.get('parent')
+ if not parent:
+ return
+ if parent == self.instance:
+ raise forms.ValidationError("Page can't be its own parent")
+ # The parent can't be part of the same tree.
+ # Retrieve all the descendants from the root of this instance.
+ # If any of the ancestor of the parent are part of this tree
+ # the parent will show up when walking the root of he instance tree.
@andrewhayward

andrewhayward Mar 4, 2013

Member

the root of the instance tree

@andrewhayward @rossbruniges andrewhayward Adjusting Page model to allow more flexibility
* `path` no longer needs to be unique
* `real_path` stores the actual requested path
* `parent` allows for the building of path structures
dc8b959
Member

rossbruniges commented Mar 4, 2013

Updated - to use real_path intead of path as the page-ID.

This means that the global nav won't get ruined if we have a nested page with the same path as a root level page.

Member

rossbruniges commented Mar 5, 2013

Closing - as try as I might I couldn't get this to work. The utils just didn't want to play ball....

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