notes

aircox/models/program.py

@@ -143,6 +143,9 @@ class Program(Page):
     def save(self, *kargs, **kwargs):
         from .sound import Sound
 
+        # FIXME: can be moved here to avoid multiple database hits
+        self.set_group_ownership()
+
         super().save(*kargs, **kwargs)
 
         # TODO: move in signals
@@ -158,9 +161,6 @@ class Program(Page):
             shutil.move(abspath, self.abspath)
             Sound.objects.filter(path__startswith=path_).update(file=Concat("file", Substr(F("file"), len(path_))))
 
-        self.set_group_ownership()
-        super().save(*kargs, **kwargs)
-
 
 class ProgramChildQuerySet(PageQuerySet):
     def station(self, station=None, id=None):

aircox/templatetags/aircox.py

@@ -59,8 +59,12 @@ def do_get_tracks(obj):
 @register.simple_tag(name="has_perm", takes_context=True)
 def do_has_perm(context, obj, perm, user=None, simple=False):
     """Return True if ``user.has_perm('[APP].[perm]_[MODEL]')``"""
+    # FIXME: is there any case when this method is used without obj?
+    #        => here we assume this shouldn't happen: obj has no default value,
+    #        If it still required, you should return a boolean value in order
+    #        to keep the same signature and contract with the function.
     if not obj:
-        return
+        return False
     if user is None:
         user = context["request"].user
     if simple:
@@ -104,8 +108,9 @@ def do_player_live_attr(context):
 @register.simple_tag(name="nav_items", takes_context=True)
 def do_nav_items(context, menu, **kwargs):
     """Render navigation items for the provided menu name."""
-    if not getattr(context["request"], "station"):
-        return []
+    # This should not happen
+    # if not getattr(context["request"], "station"):
+    #    return []
     station, request = context["station"], context["request"]
     return [(item, item.render(request, **kwargs)) for item in station.navitem_set.filter(menu=menu)]
 

aircox/tests/test_program.py

@@ -7,6 +7,7 @@ from django.core.files.uploadedfile import SimpleUploadedFile
 from aircox.models import Program
 
 
+# FIXME: better to be put in an external file, eventually load only once in tests.conftest
 png_content = (
     b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01\x08\x02\x00\x00\x00\x90wS\xde"
     + b"\x00\x00\x00\x0cIDATx\x9cc`\xf8\xcf\x00\x00\x02\x02\x01\x00{\t\x81x\x00\x00\x00\x00IEND\xaeB`\x82"
@@ -58,6 +59,8 @@ def test_edit_tracklist(user, client, program, episode, tracks):
     tracklist = [t.id for t in episode.track_set.all().order_by("position")]
     tracklist_details_reversed = [(t.id, t.artist, t.title) for t in episode.track_set.all().order_by("-position")]
     tracklist_details_reversed = list(chain(*tracklist_details_reversed))
+    # FIXME: for this too. If you wan't it cleaner
+    # also use `.format` instead of `%`.
     data = """{"website": [""], "content": ["foobar"], "new_podcast": [""], "form-TOTAL_FORMS": ["3"],
     "form-INITIAL_FORMS": ["3"], "form-MIN_NUM_FORMS": ["0"], "form-MAX_NUM_FORMS": ["1000"], "form-0-position": ["0"],
     "form-0-id": ["%s"], "form-0-": ["", "", "", "", "", ""], "form-0-artist": ["%s"], "form-0-title": ["%s"],
@@ -70,4 +73,9 @@ def test_edit_tracklist(user, client, program, episode, tracks):
     )
     r = client.post(reverse("episode-edit", kwargs={"pk": episode.pk}), json.loads(data), follow=True)
     assert r.status_code == 200
-    assert [t.id for t in episode.track_set.all().order_by("position")] == list(reversed(tracklist))
+    # FIXME: u can use set and values_list for comparison. It might be possible in this case that `all()` is not
+    # required.
+    # also you can create directly tracklist as a set: tracklist = set(episode.track_set.all().order_by("position"))
+    # => insertion order is preserved in python 3.*
+    assert set(episode.track_set.all().values_list("id", flat=True)) == set(tracklist)
+    # assert [t.id for t in episode.track_set.all().order_by("position")] == list(reversed(tracklist))

aircox/views/episode.py

@@ -53,6 +53,7 @@ class PodcastListView(EpisodeListView):
     queryset = Episode.objects.published().with_podcasts().order_by("-pub_date")
 
 
+# FIXME: to move in aircox.forms
 class EpisodeForm(ModelForm):
     content = RichTextField()
     new_podcast = FileField(required=False)

aircox/views/page.py

@@ -16,6 +16,8 @@ __all__ = [
     "BasePageDetailView",
     "PageDetailView",
     "PageListView",
+    # FIXME: don't forget to export public members of the module ;)
+    "PageUpdateView",
 ]
 
 

aircox/views/profile.py

@@ -4,12 +4,13 @@ from django.template.response import TemplateResponse
 from aircox.models import Program
 
 
+# Note: this is always better to use class-based view, even for a simple templated
+# one. There is aircox.views.View class that provides the current station and other utils.
+
+
 @login_required
 def profile(request):
-    programs = []
-    ugroups = request.user.groups.all()
-    for p in Program.objects.all():
-        if p.editors in ugroups:
-            programs.append(p)
+    groups = request.user.groups.all()
+    programs = Program.objects.filter(editors__in=groups)
     context = {"user": request.user, "programs": programs}
     return TemplateResponse(request, "accounts/profile.html", context)

aircox/views/program.py

@@ -12,7 +12,7 @@ from ..models import Article, Episode, Page, Program, StaticPage
 from .mixins import ParentMixin
 from .page import PageDetailView, PageListView, PageUpdateView
 
-__all__ = ["ProgramPageDetailView", "ProgramDetailView", "ProgramPageListView"]
+__all__ = ["ProgramPageDetailView", "ProgramDetailView", "ProgramPageListView", "ProgramUpdateView"]
 
 
 class BaseProgramMixin:
@@ -56,6 +56,7 @@ class ProgramDetailView(BaseProgramMixin, PageDetailView):
         return super().get_template_names() + ["aircox/program_detail.html"]
 
 
+# FIXME: forms should be put in aircox.forms
 class ProgramForm(ModelForm):
     content = RichTextField()
     new_cover = ImageField(required=False)
@@ -76,9 +77,7 @@ class ProgramUpdateView(UserPassesTestMixin, BaseProgramMixin, PageUpdateView):
     model = Program
     form_class = ProgramForm
 
-    def get_sidebar_queryset(self):
-        return super().get_sidebar_queryset().filter(parent=self.program)
-
+    # FIXME: ???
     def test_func(self):
         program = self.get_object()
         return self.request.user.has_perm("aircox.%s" % program.change_permission_codename)

dev-1.0-121-notes.md

@@ -0,0 +1,5 @@
+- nav_items: this method should be called only if there is a station in the context.
+- idea behind the station is that a station is always defined, except in one case: just right the platform is installed. Otherwise the idea is that user can eventually select the station through a dropbox in the nav menu (happened in last versions of aircox in the admin, should still be there).
+- don't forget to add your exported module member name to __all__ ;)
+- it is better to put forms inside aircox.forms submodule (as aircox.forms.program subfiles, etc. if the file is to big + reexport in aircox.forms.__init__)
+- you can eventually regroup urls under "/admin" or sth like this, in order to avoid mixing public and backoffice urls.