Skip to content

Conversation

@shn-liqd
Copy link
Contributor

@shn-liqd shn-liqd commented Sep 1, 2025

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@shn-liqd shn-liqd self-assigned this Sep 1, 2025
@shn-liqd shn-liqd force-pushed the so-202508-add-notifications branch from f53a07e to 187ecd3 Compare September 8, 2025 07:25
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the abstraction approach is good. However, here are some important points:

  • Make a simpler file structure, because using same folder names as the app names (projects, comments, etc) is prone to import errors and confusing. I highly recommend keeping all these new notification classes in just two files -> the strategies/base.py and strategies/mixin.py - see my inline comment on the project.py about mixin vs inheritance.
  • Keep all signals in one file too, and the helpers.py move it under the notifications folder. for same reason of keeping the overall file structure easier to navigate.
  • Do the inheritance from object, and not from ABC, as object is a more standard way to work with models, than just abstract methods in django.
  • Consider removing the Strategy suffix off the class names, as it only make the names longer without adding semantic value.
  • Verify the correct parameters are passed via the helper function _create_notifications for the strategies
  • Include in the docs, the custom template tag usage, and why is it useful.

@shn-liqd shn-liqd force-pushed the so-202508-add-notifications branch 4 times, most recently from e24730f to 57e2eb2 Compare October 6, 2025 09:52
@shn-liqd shn-liqd force-pushed the so-202508-add-notifications branch from 8ca7f92 to 4e8c9db Compare October 22, 2025 14:35
@shn-liqd shn-liqd force-pushed the so-202508-add-notifications branch from 6d39068 to 95804ff Compare November 4, 2025 09:02
@shn-liqd shn-liqd force-pushed the so-202508-add-notifications branch 2 times, most recently from de10778 to 71b5e79 Compare November 19, 2025 11:44
@shn-liqd shn-liqd force-pushed the so-202508-add-notifications branch from 71b5e79 to 85bac14 Compare November 24, 2025 08:16
@shn-liqd shn-liqd marked this pull request as ready for review November 25, 2025 08:35
@shn-liqd shn-liqd force-pushed the so-202508-add-notifications branch from 5e503d6 to 01d502f Compare December 4, 2025 08:53
@shn-liqd shn-liqd enabled auto-merge (rebase) December 4, 2025 09:08
@shn-liqd shn-liqd disabled auto-merge December 4, 2025 09:08
@shn-liqd shn-liqd enabled auto-merge (squash) December 4, 2025 09:08
@shn-liqd shn-liqd merged commit 32a6217 into main Dec 4, 2025
2 checks passed
@shn-liqd shn-liqd deleted the so-202508-add-notifications branch December 4, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants