#157 Serve Catalog Page from Wagtail #411

ahmed-arbisoft · 2019-05-30T13:53:46Z

Pre-Flight checklist

Migrations
- Migration is backwards-compatible with current production code
Testing
- Changes have been manually tested

What are the relevant tickets?

Closes #157
Closes #378

What's this PR do?

Simplifies catalog page structure. Removes extraneous html markup. Configures wagtail to server the catalog page.

How should this be manually tested?

Go to the catalog page by clicking the link in the header or going to the path /catalog/ in your deployment. The page should appear as per specifications.

asadiqbal08 · 2019-05-30T17:52:20Z

@ahmed-arbisoft you need to rebase it. I have looked into the PR, it looks good me. 👍

codecov-io · 2019-05-30T20:53:10Z

Codecov Report

Merging #411 into master will increase coverage by 0.02%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
+ Coverage   94.32%   94.35%   +0.02%     
==========================================
  Files         202      202              
  Lines        6560     7105     +545     
  Branches      370      439      +69     
==========================================
+ Hits         6188     6704     +516     
- Misses        301      323      +22     
- Partials       71       78       +7

Impacted Files	Coverage Δ
mitxpro/urls.py	`78.57% <ø> (-1.43%)`	⬇️
cms/factories.py	`100% <100%> (ø)`	⬆️
cms/models.py	`94.95% <87.5%> (+0.73%)`	⬆️
courses/views.py	`84.21% <0%> (-15.79%)`	⬇️
courses/models.py	`91.11% <0%> (-2.97%)`	⬇️
...ic/js/containers/pages/admin/BulkEnrollmentPage.js	`93.33% <0%> (-1.67%)`	⬇️
ecommerce/views.py	`99.5% <0%> (-0.5%)`	⬇️
static/js/lib/ecommerce.js	`100% <0%> (ø)`	⬆️
static/js/lib/ecommerce_test.js	`100% <0%> (ø)`	⬆️
static/js/components/forms/EmailForm.js	`100% <0%> (ø)`	⬆️
... and 12 more

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 cf2b3e5...74ab561. Read the comment docs.

gsidebo · 2019-05-31T16:02:14Z

@pdpinch Do we need to be able to link directly to the programs or courses tab in the catalog? To put it another way: If a user is on the course catalog with the "Programs" tab selected and reloads the page, is it important that the user remain on the "Programs" tab? If so, do you have any strong opinions about what the path should be, e.g.: /programs/ or /catalog/?tab=programs?

gsidebo

courses.views.CourseCatalogView should be removed, shouldn't it?

gsidebo · 2019-05-31T15:55:01Z

cms/models.py

+                "HUBSPOT_NEW_COURSES_FORM_GUID"
+            ),
+        )
+        return context


Looks like you can remove the super() call on the first line, and this return can be changed to:

return dict( **super().get_context(request), **get_js_settings_context(request), program_pages=program_pages, course_pages=course_pages, default_image_path=DEFAULT_COURSE_IMG_PATH, hubspot_portal_id=settings.HUBSPOT_CONFIG.get("HUBSPOT_PORTAL_ID"), hubspot_new_courses_form_guid=settings.HUBSPOT_CONFIG.get( "HUBSPOT_NEW_COURSES_FORM_GUID" ), )

gsidebo · 2019-05-31T16:05:37Z

cms/templates/partials/catalog-card.html

+        </div>
+    </div>
+    <div class="bottom">
+      <input type="checkbox" class="toggle" id="collapsible_{{ tab }}_{{ object_type }}_{{ courseware_page.pk }}" data-target="#collapse-{{ tab }}-{{ object_type }}-{{ courseware_page.pk }}" data-toggle="collapse" />


Seems like we should use the 'with' templatetag to define these complex id's rather than redefining them. Also, wouldn't it be sufficient just to define them as something like collapsible_{{ courseware_page.pk }}? Shouldn't the pk value be unique for each object?

Updating to collapsible_{{ tab}}_{{ coursewate_page.pk }} as the same page can appear in another tab and we don't want to be toggling in both tabs at once.

gsidebo · 2019-05-31T16:06:27Z

cms/templates/partials/catalog-card.html

+            <ul class="program-course-links">
+              {% for course_page in courseware_page.course_pages %}
+                <li>
+                  <a href="{% pageurl course_page %}" target="_blank">


Was there a requirement that these links open in a new tab? Same comment for the link at line 82

Not really, it was requested by @asadiqbal08 do you want me to remove it?

NVM, I just went ahead and removed it, was creating too many tabs anyway.

gsidebo · 2019-05-31T16:14:20Z

courses/views_test.py

+        course=CourseFactory.create(program=program, live=True)
+    )
+    course_page_no_program = CoursePageFactory.create(
+        course=CourseFactory.create(no_program=True, live=True)


This is a good change, but I think the create calls can be simplified:

course_page_in_program = CoursePageFactory.create( course__program=program, course__live=True ) course_page_no_program = CoursePageFactory.create( course__no_program=True, course__live=True )

gsidebo · 2019-05-31T16:15:12Z

courses/views_test.py

-    ProgramPageFactory.create(program=program)
-    CoursePageFactory.create(course=course_in_program)
-    CoursePageFactory.create(course=course_no_program)
+    program_page = ProgramPageFactory.create(program=program)


We can remove the comment above

gsidebo · 2019-05-31T16:15:27Z

courses/views_test.py

+        catalog_page = CatalogPage(title="Catalog", slug="catalog")
+        catalog_page = Site.objects.get(is_default_site=True).root_page.add_child(
+            catalog_page, "last-child"
+        )


Please create a fixture for (a) the root page (Site.objects.get(is_default_site=True).root_page), and (b) the catalog page (lines 210 to here). The catalog page fixture would user the root page fixture, then this test case would use the catalog page fixture. You can just define them at the top of this module

gsidebo · 2019-05-31T16:16:08Z

mitxpro/urls_test.py

@@ -1,12 +1,7 @@
 """Tests for URLs"""

 from unittest import TestCase
-from django.urls import reverse


 class URLTests(TestCase):
    """URL tests"""


This whole file can be deleted. It's not doing anything for us

pdpinch · 2019-06-03T13:07:25Z

@gsidebo asked:

Do we need to be able to link directly to the programs or courses tab in the catalog? To put it another way: If a user is on the course catalog with the "Programs" tab selected and reloads the page, is it important that the user remain on the "Programs" tab? If so, do you have any strong opinions about what the path should be, e.g.: /programs/ or /catalog/?tab=programs?

I think the current implementation, with one /catalog/ URL, is fine, unless it's somehow simpler to create unique URLs for each tab.

gsidebo

Last thing

gsidebo · 2019-06-03T18:30:02Z

cms/templates/partials/catalog-card.html

+    </div>
+    <div class="bottom">
+      {% with page_id=courseware_page.pk|stringformat:"i" %}
+        {% with collapse_id="collapsible_"|add:tab|add:"_"|add:page_id|add:page_id collapse_input_id="collapse_"|add:tab|add:page_id %}


Looks like you're adding page_id twice to the first var: add:page_id|add:page_id

The collapse_input_id value should have an underscore between tab and page_id

Any reason we can't combine these definitions with the page_id definition above to make one with block instead of 2?

That' a typo, will fix it in the next commit

Will update.

The tab variable is string and courseware_page.pk is an int. Using both in the same chain of add filter does not play nice with string concatenation so we have to convert it to a string before we can use it.

gsidebo

Looks great! 👍

Missed change Rebase and Migration Fixes Feedback collapsible tags fix Fix typo and feedback

odlbot temporarily deployed to xpro-ci-pr-411 May 30, 2019 13:54 Inactive

ahmed-arbisoft assigned tasawernawaz and asadiqbal08 May 30, 2019

odlbot deployed to xpro-ci-pr-411 May 30, 2019 19:10 View deployment

ahmed-arbisoft force-pushed the ahmedbelal/157-serve-catalog-page-wagtail branch from 77447ea to 005b9bf Compare May 30, 2019 20:53

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

odlbot had a problem deploying to xpro-ci-pr-411 May 30, 2019 21:11 Failure

gsidebo suggested changes May 31, 2019

View reviewed changes

gsidebo added the Waiting on Author label May 31, 2019

odlbot had a problem deploying to xpro-ci-pr-411 June 2, 2019 16:02 Failure

odlbot had a problem deploying to xpro-ci-pr-411 June 2, 2019 16:28 Failure

ahmed-arbisoft added Needs Review and removed Waiting on Author labels Jun 2, 2019

ahmed-arbisoft assigned gsidebo Jun 2, 2019

gsidebo suggested changes Jun 3, 2019

View reviewed changes

gsidebo added Waiting on Author and removed Needs Review labels Jun 3, 2019

odlbot had a problem deploying to xpro-ci-pr-411 June 5, 2019 00:09 Failure

ahmed-arbisoft added Needs Review and removed Waiting on Author labels Jun 5, 2019

gsidebo approved these changes Jun 5, 2019

View reviewed changes

#157 Serve Catalog Page from Wagtail

74ab561

Missed change Rebase and Migration Fixes Feedback collapsible tags fix Fix typo and feedback

ahmed-arbisoft force-pushed the ahmedbelal/157-serve-catalog-page-wagtail branch from 6073c32 to 74ab561 Compare June 5, 2019 14:37

odlbot had a problem deploying to xpro-ci-pr-411 June 5, 2019 14:37 Failure

ahmed-arbisoft merged commit 7eb87d0 into master Jun 5, 2019

ahmed-arbisoft deleted the ahmedbelal/157-serve-catalog-page-wagtail branch June 5, 2019 14:50

ahmed-arbisoft mentioned this pull request Jun 5, 2019

Course Catalog: Program links don't work #379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#157 Serve Catalog Page from Wagtail #411

#157 Serve Catalog Page from Wagtail #411

ahmed-arbisoft commented May 30, 2019 •

edited

Loading

asadiqbal08 commented May 30, 2019

codecov-io commented May 30, 2019 •

edited

Loading

gsidebo commented May 31, 2019

gsidebo left a comment

gsidebo May 31, 2019

ahmed-arbisoft Jun 2, 2019

gsidebo May 31, 2019

ahmed-arbisoft Jun 2, 2019

gsidebo May 31, 2019

ahmed-arbisoft Jun 2, 2019

ahmed-arbisoft Jun 2, 2019

gsidebo May 31, 2019

ahmed-arbisoft Jun 2, 2019

gsidebo May 31, 2019

ahmed-arbisoft Jun 2, 2019

gsidebo May 31, 2019

ahmed-arbisoft Jun 2, 2019

gsidebo May 31, 2019

ahmed-arbisoft Jun 2, 2019

pdpinch commented Jun 3, 2019

gsidebo left a comment

gsidebo Jun 3, 2019 •

edited

Loading

ahmed-arbisoft Jun 5, 2019

ahmed-arbisoft Jun 5, 2019

gsidebo left a comment

#157 Serve Catalog Page from Wagtail #411

#157 Serve Catalog Page from Wagtail #411

Conversation

ahmed-arbisoft commented May 30, 2019 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

asadiqbal08 commented May 30, 2019

codecov-io commented May 30, 2019 • edited Loading

Codecov Report

gsidebo commented May 31, 2019

gsidebo 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

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

pdpinch commented Jun 3, 2019

gsidebo left a comment

Choose a reason for hiding this comment

gsidebo Jun 3, 2019 • edited Loading

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 30, 2019 •

edited

Loading

codecov-io commented May 30, 2019 •

edited

Loading

gsidebo Jun 3, 2019 •

edited

Loading