- Addressed some feedback from PR Review

Author: ehmadzubair

home/forms.py

@@ -1,15 +1,14 @@
+from django.apps import apps
 from wagtail.admin.forms import WagtailAdminPageForm
 
-from home.utils.progress import get_progress_bar_eligible_sections
-
-
 class SectionPageForm(WagtailAdminPageForm):
 
     def clean(self):
         cleaned_data = super().clean()
 
         if cleaned_data['show_progress_bar']:
-            if self.instance not in get_progress_bar_eligible_sections():
+            Section = apps.get_model('home', 'Section')
+            if self.instance not in Section.get_progress_bar_eligible_sections():
                 progress_bar_enabled_ancestor_title = self.instance.get_progress_bar_enabled_ancestor().title
                 self.add_error(
                     'show_progress_bar',

home/migrations/0022_merge_20210629_0821.py

@@ -0,0 +1,14 @@
+# Generated by Django 3.1.12 on 2021-06-29 08:21
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('home', '0019_section_show_progress_bar'),
+        ('home', '0021_auto_20210628_1107'),
+    ]
+
+    operations = [
+    ]

home/models.py

@@ -1,8 +1,11 @@
+from django.contrib.admin.utils import flatten
+from django.contrib.auth import get_user_model
 from django.db import models
 from django.utils.encoding import force_str
 from django.utils.translation import gettext_lazy as _
 
 from modelcluster.contrib.taggit import ClusterTaggableManager
+from rest_framework import status
 from taggit.models import TaggedItemBase
 from modelcluster.fields import ParentalKey
 from wagtail.admin.edit_handlers import (
@@ -30,6 +33,7 @@
 from .forms import SectionPageForm
 from .utils.progress_manager import ProgressManager
 
+User = get_user_model()
 
 class HomePage(Page):
     template = 'home/home_page.html'
@@ -134,10 +138,11 @@ class Section(Page):
     base_form_class = SectionPageForm
 
     def get_descendant_articles(self):
-        return Article.objects.descendant_of(self).type(Article)
+        return Article.objects.descendant_of(self).exact_type(Article)
 
     def get_progress_bar_enabled_ancestor(self):
-        return Section.objects.ancestor_of(self, inclusive=True).type(Section).filter(show_progress_bar=True).first()
+        return Section.objects.ancestor_of(self, inclusive=True).exact_type(Section).filter(
+            show_progress_bar=True).first()
 
     def get_user_progress_dict(self, request):
         progress_manager = ProgressManager(request)
@@ -162,6 +167,22 @@ def get_context(self, request):
 
         return context
 
+    @staticmethod
+    def get_progress_bar_eligible_sections():
+        """
+        Eligibility criteria:
+        Sections whose ancestors don't have show_progress_bar=True are eligible to
+        show progress bars.
+        :return:e
+        """
+        progress_bar_sections = Section.objects.filter(show_progress_bar=True)
+        all_descendants = [list(Section.objects.type(Section).descendant_of(section).values_list('pk', flat=True)) for
+                           section in
+                           progress_bar_sections]
+        all_descendants = set(flatten(all_descendants))
+
+        return Section.objects.exclude(pk__in=all_descendants)
+
 
 class ArticleRecommendation(Orderable):
     source = ParentalKey('Article', related_name='recommended_articles', on_delete=models.CASCADE, blank=True)
@@ -273,8 +294,8 @@ def get_context(self, request):
 
     def serve(self, request):
         response = super().serve(request)
-        progress_manager = ProgressManager(request)
-        progress_manager.record_article_read(self)
+        if response.status_code == status.HTTP_200_OK:
+            User.record_article_read(request=request, article=self)
         return response
 
     def description(self):

home/templates/home/tags/section_progress.html

@@ -1,7 +1,7 @@
-<div class="user-progress-container">
-    {% if total %}
+{% if total %}
+    <div class="user-progress-container">
         <div>
             User Progress: {{ read }}/{{ total }} completed
         </div>
-    {% endif %}
-</div>
+    </div>
+{% endif %}

home/utils/progress.py

@@ -3,29 +3,12 @@ class ProgressManager:
     def __init__(self, request):
         self.request = request
 
-    def record_article_read(self, article):
-        user = self.request.user
-        if user.is_anonymous:
-            read_articles = self.request.session.get('read_articles', [])
-            if read_articles:
-                if article.pk not in read_articles:
-                    # https://code.djangoproject.com/wiki/NewbieMistakes#Appendingtoalistinsessiondoesntwork
-                    read_articles = self.request.session['read_articles']
-                    read_articles.append(article.pk)
-                    self.request.session['read_articles'] = read_articles
-            else:
-                self.request.session['read_articles'] = [article.pk]
-        else:
-            if not user.viewed_articles.filter(id=article.id).exists():
-                user.viewed_articles.add(article)
-
-
     def get_progress(self, section):
         user = self.request.user
         if self.request.user.is_anonymous:
             read_article_ids = set(self.request.session.get('read_articles', []))
         else:
-            read_article_ids = set(user.viewed_articles.values_list('pk', flat=True))
+            read_article_ids = set(user.read_articles.values_list('pk', flat=True))
 
         progress_enabled_ancestor = section.get_progress_bar_enabled_ancestor()
         if progress_enabled_ancestor:

home/utils/progress_manager.py

@@ -1,19 +1,19 @@
-# Generated by Django 3.1.12 on 2021-06-28 17:21
+# Generated by Django 3.1.12 on 2021-06-29 08:23
 
 from django.db import migrations, models
 
 
 class Migration(migrations.Migration):
 
     dependencies = [
-        ('home', '0019_section_show_progress_bar'),
+        ('home', '0022_merge_20210629_0821'),
         ('iogt_users', '0002_auto_20210624_0739'),
     ]
 
     operations = [
         migrations.AddField(
             model_name='user',
-            name='viewed_articles',
+            name='read_articles',
             field=models.ManyToManyField(to='home.Article'),
         ),
     ]

iogt_users/migrations/0003_user_viewed_articles.py iogt_users/migrations/0003_user_read_articles.py

@@ -13,7 +13,24 @@ class User(AbstractUser):
 
     has_filled_registration_survey = models.BooleanField(default=False)
 
-    viewed_articles = models.ManyToManyField(to='home.Article')
+    read_articles = models.ManyToManyField(to='home.Article')
+
+    @classmethod
+    def record_article_read(cls, request, article):
+        user = request.user
+        if user.is_anonymous:
+            read_articles = request.session.get('read_articles', [])
+            if read_articles:
+                if article.pk not in read_articles:
+                    # https://code.djangoproject.com/wiki/NewbieMistakes#Appendingtoalistinsessiondoesntwork
+                    read_articles = request.session['read_articles']
+                    read_articles.append(article.pk)
+                    request.session['read_articles'] = read_articles
+            else:
+                request.session['read_articles'] = [article.pk]
+        else:
+            if not user.read_articles.filter(id=article.id).exists():
+                user.read_articles.add(article)
 
     class Meta:
         ordering = ('id',)