#155 Integrate Wagtail Routing #395

ahmed-arbisoft · 2019-05-28T19:17:56Z

Pre-Flight checklist

Screenshots of screens that were affected
- Desktop screenshots
- Tag @Ferdi or @pdpinch for review
Migrations
- Migration is backwards-compatible with current production code
Testing
- Code is tested
- Changes have been manually tested

What are the relevant tickets?

Closes #155

What's this PR do?

Adds a migration to move course and program pages to their respective index pages under the home page.
Moves all properties previously being accessed from Course/Program to CoursePage/ProgramPage
Moves all page related tests from courses/models_test to cms/models_test
Moves templates from courses/templates to cms/templates
Modifies partials to use CoursePage/ProgramPage as the source of information
Updates serializers and serializer_tests to use the new setup (CoursePage/ProgramPage for source of most information)
Adds product_page template for Wagtail to use in order to serve program and course pages
Modifies all URL generation code to generate wagtail compatible links for the product pages

How should this be manually tested?

Run the migrations, you should end up with a "Courses" page and "Programs" page under the home page, of the types CourseIndexPage and ProgramIndexPage respectively. All your courses and programs should have been moved to these index pages by the migration. In the CMS, go to any CoursePage or ProgramPage and click on the live link. The page should be rendered as expected, with the url pattern for both being /courses/<slug>/ and /programs/<slug>/ respectively.

Screenshots (if appropriate)

codecov-io · 2019-05-28T19:20:27Z

Codecov Report

Merging #395 into master will increase coverage by 0.05%.
The diff coverage is 95.71%.

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
+ Coverage   94.19%   94.24%   +0.05%     
==========================================
  Files         192      192              
  Lines        6386     6378       -8     
  Branches      364      361       -3     
==========================================
- Hits         6015     6011       -4     
+ Misses        303      299       -4     
  Partials       68       68

Impacted Files	Coverage Δ
courses/urls.py	`100% <ø> (ø)`	⬆️
courses/models.py	`94.07% <ø> (-0.64%)`	⬇️
courses/serializers.py	`100% <100%> (ø)`	⬆️
courses/views.py	`100% <100%> (ø)`	⬆️
cms/models.py	`94.22% <94.64%> (+2.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35a6ff5...724440c. Read the comment docs.

rhysyngsun

I'm a bit out of the loop as to what is intended with this change so I just left general feedback and will leave it to @gsidebo to go over the wagtail-specifics since he's been involved more in that.

rhysyngsun · 2019-05-30T15:56:16Z

cms/management/commands/setup_index_pages.py

+        delete = options["revert"]
+        site = Site.objects.get(is_default_site=True)
+        if not site:
+            print(


Suggested change

print(

self.stderr.write(

rhysyngsun · 2019-05-30T15:56:49Z

cms/management/commands/setup_index_pages.py

+            print(
+                "No site setup. Please configure a default site before running this command"
+            )
+            return


Can you have this exit with a non-zero exit code?

rhysyngsun · 2019-05-30T15:58:47Z

cms/migrations/0028_setup_course_program_index_pages.py

+def get_top_level_wagtail_page():
+    """
+    The Wagtail CMS (at least in our usage) has one root page at depth 1, and one page at depth 2. All pages that we
+    create in Wagtail are added as children to the page at depth 2.


How is this enforced? If this isn't the case in all environments then this migration will fail.

It's technically possible for there to be multiple pages at depth 2, for example if in some environment we have the original "This is your Wagtail site!" page existing alongside the new Home Page. It wouldn't hurt to update this query to fetch all pages at depth 2. If there's just one, return that. If there's more than one, pick the Home Page.

The function name and docstring can also be updated.

~~get_top_level_wagtail_page~~ -> get_home_page

docstring: The Wagtail CMS (at least in our usage) has one uneditable root page at depth 1, and a home page as its child at depth 2

Rather than going through pages at depth 1 or 2, I'm changing this to use the Site model to fish out the configured home page ID. That gets rid of all assumptions.

Not going through the depth 1/2 route because you can in fact have multiple site records as well, so that's also a potential point of failure.

rhysyngsun · 2019-05-30T16:05:46Z

ecommerce/serializers.py

@@ -22,6 +22,27 @@
 from mitxpro.serializers import WriteableSerializerMethodField


+def _get_thumbnail_url(page):


Don't duplicate this method

Removed, was left over from some fixes earlier, isn't being used anyway. Oversight on my part.

gsidebo · 2019-05-30T18:39:51Z

cms/models.py

+        if not self.program_page:
+            return []
+        courses = self.program_page.program.courses.all()
+        return CoursePage.objects.filter(course_id__in=courses)


Pretty sure this function can be rewritten as:

return CoursePage.objects.filter(course__program=self.course.program) if self.course.program else []

gsidebo · 2019-05-30T18:55:15Z

cms/migrations/0028_setup_course_program_index_pages.py

+def get_top_level_wagtail_page():
+    """
+    The Wagtail CMS (at least in our usage) has one root page at depth 1, and one page at depth 2. All pages that we
+    create in Wagtail are added as children to the page at depth 2.


It's technically possible for there to be multiple pages at depth 2, for example if in some environment we have the original "This is your Wagtail site!" page existing alongside the new Home Page. It wouldn't hurt to update this query to fetch all pages at depth 2. If there's just one, return that. If there's more than one, pick the Home Page.

The function name and docstring can also be updated.

~~get_top_level_wagtail_page~~ -> get_home_page

docstring: The Wagtail CMS (at least in our usage) has one uneditable root page at depth 1, and a home page as its child at depth 2

gsidebo · 2019-05-30T18:58:33Z

cms/models.py

@@ -527,6 +566,7 @@ class Meta:

    def get_context(self, request, *args, **kwargs):
        context = super(ProductPage, self).get_context(request)
+        context.update(**get_js_settings_context(request))


This function can be rewritten as:

return { **super().get_context(request, *args, **kwargs), **get_js_settings_context(request), "title": self.title }

gsidebo · 2019-05-30T20:24:52Z

cms/models.py

@@ -607,7 +705,55 @@ def program_page(self):
        """
        Gets the program page associated with this course, if it exists
        """
-        return self.course.program.page if self.course and self.course.program else None
+        return self.course.program.page if self.course.program.page else None


This needs to be changed. As written, it will not work for courses with no program. Change to:

return self.course.program.page if self.course.program else None

gsidebo

👍

odlbot deployed to xpro-ci-pr-395 May 28, 2019 19:18 View deployment

odlbot had a problem deploying to xpro-ci-pr-395 May 28, 2019 19:20 Failure

ahmed-arbisoft assigned tasawernawaz, asadiqbal08 and gsidebo May 28, 2019

ahmed-arbisoft added the Needs Review label May 28, 2019

ahmed-arbisoft mentioned this pull request May 28, 2019

#383 Add Home Page Instructions to Readme #388

Merged

tasawernawaz mentioned this pull request May 30, 2019

Fix missing price on product page #409

Merged

rhysyngsun requested changes May 30, 2019

View reviewed changes

gsidebo mentioned this pull request May 30, 2019

Integrated Wagtail to support course/program detail pages #264

Closed

7 tasks

gsidebo suggested changes May 30, 2019

View reviewed changes

Ahmed Belal added 4 commits May 31, 2019 00:48

#155 Integrate Wagtail Routing

28e6d04

Removed unused import

a949257

Feedback

9713ed4

Rebase + Migration Conflict Fixes

0774ab7

ahmed-arbisoft force-pushed the ahmedbelal/155-integrate-wagtail-routing-serving branch from d2371f7 to 0774ab7 Compare May 30, 2019 19:50

odlbot had a problem deploying to xpro-ci-pr-395 May 30, 2019 19:50 Failure

gsidebo suggested changes May 30, 2019

View reviewed changes

Feedback

724440c

odlbot had a problem deploying to xpro-ci-pr-395 May 30, 2019 20:39 Failure

gsidebo approved these changes May 30, 2019

View reviewed changes

ahmed-arbisoft merged commit f3f5af3 into master May 30, 2019

tasawernawaz mentioned this pull request May 31, 2019

Programs metadata in metadata-tiles.html. #410

Closed

ahmed-arbisoft deleted the ahmedbelal/155-integrate-wagtail-routing-serving branch July 10, 2019 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#155 Integrate Wagtail Routing #395

#155 Integrate Wagtail Routing #395

ahmed-arbisoft commented May 28, 2019 •

edited

Loading

codecov-io commented May 28, 2019 •

edited

Loading

rhysyngsun left a comment

rhysyngsun May 30, 2019

ahmed-arbisoft May 30, 2019

rhysyngsun May 30, 2019

ahmed-arbisoft May 30, 2019

rhysyngsun May 30, 2019

gsidebo May 30, 2019

ahmed-arbisoft May 30, 2019 •

edited

Loading

rhysyngsun May 30, 2019

ahmed-arbisoft May 30, 2019

gsidebo May 30, 2019

ahmed-arbisoft May 30, 2019

gsidebo May 30, 2019

gsidebo May 30, 2019

ahmed-arbisoft May 30, 2019

gsidebo May 30, 2019

ahmed-arbisoft May 30, 2019

gsidebo left a comment

		@@ -22,6 +22,27 @@
		from mitxpro.serializers import WriteableSerializerMethodField


		def _get_thumbnail_url(page):

#155 Integrate Wagtail Routing #395

#155 Integrate Wagtail Routing #395

Conversation

ahmed-arbisoft commented May 28, 2019 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Screenshots (if appropriate)

codecov-io commented May 28, 2019 • edited Loading

Codecov Report

rhysyngsun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmed-arbisoft May 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsidebo left a comment

Choose a reason for hiding this comment

ahmed-arbisoft commented May 28, 2019 •

edited

Loading

codecov-io commented May 28, 2019 •

edited

Loading

ahmed-arbisoft May 30, 2019 •

edited

Loading