From 722f76767e60aa43c6d391307e4304c0459e333c Mon Sep 17 00:00:00 2001 From: bkfox Date: Mon, 19 Feb 2024 23:18:48 +0100 Subject: [PATCH] notes --- aircox/models/program.py | 6 +++--- aircox/templatetags/aircox.py | 11 ++++++++--- aircox/tests/test_program.py | 10 +++++++++- aircox/views/episode.py | 1 + aircox/views/page.py | 2 ++ aircox/views/profile.py | 11 ++++++----- aircox/views/program.py | 7 +++---- dev-1.0-121-notes.md | 5 +++++ 8 files changed, 37 insertions(+), 16 deletions(-) create mode 100644 dev-1.0-121-notes.md diff --git a/aircox/models/program.py b/aircox/models/program.py index c0056df..728f670 100644 --- a/aircox/models/program.py +++ b/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): diff --git a/aircox/templatetags/aircox.py b/aircox/templatetags/aircox.py index e7c89a8..74198de 100644 --- a/aircox/templatetags/aircox.py +++ b/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)] diff --git a/aircox/tests/test_program.py b/aircox/tests/test_program.py index 8cce944..f02c523 100644 --- a/aircox/tests/test_program.py +++ b/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)) diff --git a/aircox/views/episode.py b/aircox/views/episode.py index 7e7107f..175a3cb 100644 --- a/aircox/views/episode.py +++ b/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) diff --git a/aircox/views/page.py b/aircox/views/page.py index 62869c3..2c55ed8 100644 --- a/aircox/views/page.py +++ b/aircox/views/page.py @@ -16,6 +16,8 @@ __all__ = [ "BasePageDetailView", "PageDetailView", "PageListView", + # FIXME: don't forget to export public members of the module ;) + "PageUpdateView", ] diff --git a/aircox/views/profile.py b/aircox/views/profile.py index 4e002fa..2711229 100644 --- a/aircox/views/profile.py +++ b/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) diff --git a/aircox/views/program.py b/aircox/views/program.py index e59c322..91a1f9c 100644 --- a/aircox/views/program.py +++ b/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) diff --git a/dev-1.0-121-notes.md b/dev-1.0-121-notes.md new file mode 100644 index 0000000..775b69d --- /dev/null +++ b/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.