Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Purge menu cache while creation/editing and various version state changes #38

Merged
merged 6 commits into from Mar 12, 2019

Conversation

webbyfox
Copy link
Contributor

@webbyfox webbyfox commented Mar 7, 2019

No description provided.

@webbyfox webbyfox changed the title WIP: Purge the menu cache while creation/editing and various version state changes WIP: Purge menu cache while creation/editing and various version state changes Mar 7, 2019
tests/test_plugin.py Outdated Show resolved Hide resolved
tests/test_plugin.py Show resolved Hide resolved
@webbyfox webbyfox changed the title WIP: Purge menu cache while creation/editing and various version state changes Purge menu cache while creation/editing and various version state changes Mar 8, 2019
tests/test_plugin.py Show resolved Hide resolved
# Now publish the page content containing the plugin,
# so the page can be viewed
version = page_content.versions.get()
version.publish(self.get_superuser())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, now seeing this simplified version of the test, I think you can simplify further. Possibly you can create the page content as published straight away? As in page_content = factories.PageContentWithVersionFactory(language=self.language, version__created_by=self.get_superuser(), version__state=PUBLISHED) above. Then you don't need these two lines at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure you can add a placeholder(and plugin) to the published page.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you're modifying the page after this line

cache_key = CacheKey.objects.all().count()
self.assertEqual(cache_key, 1)

self.assertEqual(response.status_code, 200)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment these asserts so it's clear what and why you're checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this assert needs any comment.

self.assertIn(child.title, str(response.content))
self.assertIn(grandchild.title, str(response.content))

# Create further menu items which covers draft state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "covers draft state" mean? And why does the draft state matter in this test? I thought this line was here simply because you want to check that when the menu version is modified, the cache clearing fires up?

@@ -261,3 +263,186 @@ def test_crud_for_navigation_plugin_when_versioning_disabled(self):

# Now delete the plugin from the page and assert
self._delete_nav_plugin_and_assert(new_placeholder, new_plugin)

def test_menu_cache_invalidate_after_menuitem_creation(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is actually a test for whether the cache is invalidated after edit rather than after creation, right?

child2 = factories.ChildMenuItemFactory(parent=menu_content.root)

# grandchild2
factories.ChildMenuItemFactory(parent=child2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, I think you only need one of these two lines.

# Add nav plugin to placeholder
self._add_nav_plugin_and_assert(
placeholder, menu_content.menu, "menu/menu.html"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, ditto on TODO comment (and same for tests below)

# Now publish the page content containing the plugin,
# so the page can be viewed
version = page_content.versions.get()
version.publish(self.get_superuser())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, you probably won't need this if you just set this version to published straight away (and ditto for tests below).

page_url = page_content.page.get_absolute_url()
response = self.client.get(page_url)

# Rendering should generate cachekey object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confused me, I thought you'd missed an intended assert, but then realized the comment actually describes an assert that is 4 lines down. So you might want to fix the line spacing/position of the comment/asserts here.

page_url = page_content.page.get_absolute_url()
response = self.client.get(page_url)

# Rendering should generate cachekey object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on strange line spacing/comment not describing the block of code it's attached to.

@webbyfox webbyfox merged commit 98a58f9 into master Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants