Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Pages update #95

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
3 participants
Member

andrewhayward commented Feb 28, 2013

This is an alternative option to #94. Rather than working out paths at request time, it stores the complete path in the database, so only needs to run one query. Seems to make more sense to do it this way round.

Also means that you can have multiple pages with the same slug, as long as the full paths don't conflict, allowing for things like:

  • /about/
  • /mentors/about/
Member

rossbruniges commented Mar 1, 2013

That certainly feels a bit more elegant than my solution :)

Wonder if you could get @alfredo to have a butchers over it too?

Thanks for the alternative - we now have options and that can't be anything but good!!!

Member

andrewhayward commented Mar 1, 2013

I think I've resolved the most significant failing this morning, which was the ancestral looping issue. That should, in theory, no longer be possible. Also cleaned up the admin page so that only viable parent pages are available, and (as an added bonus) display as per the hierarchy.

Member

rossbruniges commented Mar 1, 2013

Ancestral looping sounds like a good thing to avoid!!! What URL's are those??

One thing - can you do an interactive rebate to scrunch the commits? It's quite a ropey subject - can wait around until Monday and you're happy with the code.

Again, many thanks!!

Member

andrewhayward commented Mar 1, 2013

Yeah, ancestral looping is probably best avoided (and also an Eighties cover band, or something). Of course, there's no absolute guarantee as regards data integrity - nothing to stop database-level changes, nor indeed code level changes. I guess the second point could be resolved by adding a call to clean inside of save, but that then allows for unexpected ValidationErrors. Something to think about, I guess.

As regards commit squashing, I'll leave that until next week, when you and @alfredo have had a chance to look over the code properly, and you're sure you want to use the pull request :) Probably some glaring issues I've not yet noticed!

Member

alfredo commented Mar 3, 2013

I had a go at solving the problem.

Because running the site involves a few steps I decided for isolating the pages app and work on that. I created a quick proof of concept repo: https://github.com/alfredo/make_mozilla

I'll annotate the code where necessary.

I wrote my tests using py.test, because that was the easiest thing to use on the POC, but I think they can be copy and pasted and used with django nose.

Also more integration tests are needed for the admin bits, just unittested the core bits while walking the tree.

@alfredo alfredo commented on the diff Mar 3, 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 clean(self):
@alfredo

alfredo Mar 3, 2013

Member

I think all this functionality can be moved to the admin ModelForm. in the parent because it is related to validating only whether the parent is valid or not https://github.com/alfredo/make_mozilla/blob/master/make_mozilla/pages/admin.py#L41-L67

Also you will notice that I moved any of the bits that walked the tree to some helper functions, this makes easier to unit test the functionality without having to prepare several fixtures to test the integration.

https://github.com/alfredo/make_mozilla/blob/master/make_mozilla/pages/utils.py

@andrewhayward

andrewhayward Mar 4, 2013

Member

Yeah, personal preference, I suppose. I tend to keep this sort of logic in the model, rather than the form. This method is a bit messy though anyway, so probably needs to be looked at regardless :)

@alfredo alfredo commented on the diff Mar 3, 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 clean(self):
+ from django.core.exceptions import ValidationError
@alfredo

alfredo Mar 3, 2013

Member

It is recommended to keep the imports at the top of the file, this will avoid recursive import surprises, it is also pep8.

@andrewhayward

andrewhayward Mar 4, 2013

Member

I didn't even think about it, to be honest - just copied the stub from the Django docs and did what needed to be done! But yes, noted; this should be at the top of the file.

@rossbruniges

rossbruniges Mar 4, 2013

Member

I'm tinkering with this branch locally at the moment - will try and get the good stuff out of @andrewhayward and @alfredo stuff into it.

The potential for infinite loops seems a bit worrying but having had a brief play around I've not been able to trigger it yet. As the people using this are likely to be Matt and other MoFo staff, and that the pages output from it are pretty temporary I think it's a risk that we can take.

@andrewhayward

andrewhayward Mar 4, 2013

Member

@rossbruniges - I think the form should prevent any (obvious) problems, but feel free to write tests against it (or steal @alfredo's!) to make sure :P

@alfredo alfredo commented on the diff Mar 3, 2013

make_mozilla/pages/models.py
+ if self.parent:
+ if self.parent.has_ancestor(self):
+ raise ValidationError('Cannot set page parent to one of its descendants')
+
+ self.real_path = '%s/%s' % (self.parent.real_path, self.path)
+ else:
+ self.real_path = self.path
+
+ try:
+ if Page.objects.exclude(id__exact=self.id).get(real_path=self.real_path):
+ raise ValidationError('This path/parent combination already exists.')
+ except Page.DoesNotExist:
+ # We can safely ignore this, as it means we're in the clear and our path is fine
+ pass
+
+ def save(self, *args, **kwargs):
@alfredo

alfredo Mar 3, 2013

Member

On my implementation I decided to explicitly specify when the children should be updated. I think it generates less surprises, saving one instance that affects the record in all its children, on a standard save call.

https://github.com/alfredo/make_mozilla/blob/master/make_mozilla/pages/models.py#L33-L40

@andrewhayward

andrewhayward Mar 4, 2013

Member

Yeah, difficult that one. Did it this way so as to maintain data integrity - I'd probably make it default to True if it were optional, to be honest.

Member

alfredo commented Mar 3, 2013

I believe that all the new bits in the poc repo can be used straight away, since I didn't alter any data structure, considering the existing south-migration.

Please feel free to use / not use the code, also feedback is always welcome! :)

Member

rossbruniges commented Mar 6, 2013

This got merged in as part of #100

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