Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve static events #12

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Improve static events #12

wants to merge 6 commits into from

Conversation

fopina
Copy link
Contributor

@fopina fopina commented Jan 21, 2023

A common pattern across surface modules (and any app using this) is to use hardcoded events.

This is very likely most of the use of this package, not the external events, where it does make sense to create event manually as the trigger is also external to the code.

For the hardcoded events, the code requires the event to exist so it will usually include the data migration to create it (boring).
This change, EventRef, allows the code that creates notifications to reference the event using an instance of this class (instead of the event name: str)

There's a post_migrate signal handler that will create every event found in the code (instances of EventRef) so manual data migrations are not longer required.

Added bonus is that this invites developers to actually define the events as constants instead of using the name string everywhere, avoiding typos.

Caveat is that the EventRef variable has to be defined in some module that is imported automatically so the variable is instantiated (and registered) before the signal is received.

There's an easy choice for it though: models.py even if no models are defined in the app (admin.py is another, if django-admin is used, but models.py makes less-less sense).

Hopefully, the small diff is better explanatory.

I meant to document this but I noticed README.md is quite behind (close to non-existent) so I'll leave that to another PR!

I would prefer to have some system that would create actual migrations under the consumer app instead of running every time after migrate but I couldn't find any hooks for that and this looked like an acceptable alternative (as it only runs with migrate anyway)

No breaking changes introduced

Also, description added to events, as the name alone sometimes might not be explanatory enough (even more for manually added external events)

Feedback welcome!

(mildly related to #6)

@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2023

Codecov Report

Base: 80.80% // Head: 81.28% // Increases project coverage by +0.47% 🎉

Coverage data is based on head (a2299bd) compared to base (2f558a3).
Patch coverage: 91.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   80.80%   81.28%   +0.47%     
==========================================
  Files          13       14       +1     
  Lines         521      545      +24     
==========================================
+ Hits          421      443      +22     
- Misses        100      102       +2     
Impacted Files Coverage Δ
notifications/admin.py 61.73% <ø> (ø)
notifications/models.py 93.82% <85.71%> (-1.70%) ⬇️
notifications/apps.py 100.00% <100.00%> (ø)
notifications/migrations/0004_event_description.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fopina
Copy link
Contributor Author

fopina commented Jan 21, 2023

Just remembered that this has another caveat...
When there is an unnapplied migration, django warns about it (with any command). With this approach there's an hidden pseudo data migration that django won't warn about.

When the app is freshly installed, it shouldn't be an issue as there will be other migrations to apply (and this will be triggered as well), but if events/notifications are added later, upgrading to the new version won't warn about those.

But again, I couldn't find anythin to generate actual migrations... 🤷

tox.ini Show resolved Hide resolved
@fopina
Copy link
Contributor Author

fopina commented Feb 9, 2023

Converting this to draft so documentation is also updated before merge:

  • section on how to use from code (where this change is included)
  • section on how to use external events (be sure to include token and curl example)

@fopina fopina marked this pull request as draft February 9, 2023 00:38
Signed-off-by: Filipe Pina <[email protected]>
Signed-off-by: Filipe Pina <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Feb 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

3 participants