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

[#1760] Implemented user feed #933

Merged
merged 4 commits into from
Jan 16, 2024
Merged

[#1760] Implemented user feed #933

merged 4 commits into from
Jan 16, 2024

Conversation

Bartvaderkin
Copy link
Contributor

@Bartvaderkin Bartvaderkin commented Jan 5, 2024

Review notes:

It's a lot of files, but to get an idea of how it works first look at:

  • open_inwoner/userfeed/feed.py for the get_feed(user) that is the main entry for retrieval and display (in the plugin etc)
  • open_inwoner/userfeed/hooks/*.py for the 'hook' functions that create items of a specific type, the function to mark some as completed, and their optional 'adapter' class that generalizes the type for display by get_feed() (and the template etc)

Everything else is just glue to make it work or to tie it into the rest of the application. Note the liberal use of patch/mock in the tests to keep the user feed specific tests out of the other parts of the code.

In the future we'll add more item types to the choices/hooks/adapters.


To test the plugin put it on the homepage, and then run this command to add a test message:

python src/manage.py add_feed_message --user 1 --title "Helo World" --message "Waddup?" --url "http://www.maykinmedia.nl"

These never expire, so delete from /admin/userfeed/feeditemdata/. To find valid --user User id's use the command without arguments and it prints staff users:

python src/manage.py add_feed_message

@Bartvaderkin Bartvaderkin force-pushed the feature/1760-userfeed branch 2 times, most recently from 04f9382 to 0290024 Compare January 9, 2024 10:26
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2024

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (b3f5504) 93.11% compared to head (a2c9d69) 94.80%.
Report is 44 commits behind head on develop.

Files Patch % Lines
...r/userfeed/management/commands/add_feed_message.py 69.56% 7 Missing ⚠️
src/open_inwoner/userfeed/models.py 93.65% 4 Missing ⚠️
src/open_inwoner/userfeed/adapters.py 86.36% 3 Missing ⚠️
src/open_inwoner/openzaak/utils.py 87.50% 1 Missing ⚠️
src/open_inwoner/userfeed/adapter.py 97.05% 1 Missing ⚠️
src/open_inwoner/userfeed/feed.py 98.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #933      +/-   ##
===========================================
+ Coverage    93.11%   94.80%   +1.69%     
===========================================
  Files          829      857      +28     
  Lines        29142    30046     +904     
===========================================
+ Hits         27135    28485    +1350     
+ Misses        2007     1561     -446     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Bartvaderkin Bartvaderkin changed the title WIP [#1760] Implemented user feed [#1760] Implemented user feed Jan 9, 2024
@Bartvaderkin Bartvaderkin marked this pull request as ready for review January 9, 2024 11:37
Copy link
Contributor

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

Looks good overall 👍, just a few minor remarks/questions

src/open_inwoner/openzaak/notifications.py Outdated Show resolved Hide resolved
src/open_inwoner/userfeed/models.py Show resolved Hide resolved
src/open_inwoner/userfeed/adapters.py Outdated Show resolved Hide resolved
@@ -48,6 +49,10 @@ def handle(self, *args, **options):
f"The email was sent to the user {receiver} about {plans.count()} expiring plans"
)

# hook into userfeed
for p in plans:
plan_expiring(receiver, p)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed a pattern with the userfeed hooks: the function names are not very descriptive, which is why every time you're hooking into the userfeed you need to add comments to indicate what you're doing.

Alternatively, you could try the following pattern:

  • import the hook functions (e.g. plan_expiring) in userfeed.hooks.__init__
  • in management.commands.plans_expire:
    • from open_inwoner.userfeed import hooks as userfeed_hooks
    • for p in plans: userfeed_hooks.plan_expiring(receiver,p)
  • modify the relevant patch in test_plans_expire: @patch("open_inwoner.plans.management.commands.plans_expire.userfeed_hooks.plan_expiring")

The code would be self-explanatory, and comments would be unnecessary. The same pattern would work for the other cases (I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK this is also how some of the timelinelogs works so let's do that.

src/open_inwoner/plans/tests/test_plans_expire.py Outdated Show resolved Hide resolved
src/open_inwoner/plans/views.py Outdated Show resolved Hide resolved
src/open_inwoner/userfeed/apps.py Outdated Show resolved Hide resolved
src/open_inwoner/userfeed/feed.py Outdated Show resolved Hide resolved
src/open_inwoner/userfeed/models.py Outdated Show resolved Hide resolved
src/open_inwoner/userfeed/models.py Outdated Show resolved Hide resolved
src/open_inwoner/userfeed/summarize.py Show resolved Hide resolved
src/open_inwoner/userfeed/summarize.py Show resolved Hide resolved
src/open_inwoner/userfeed/tests/test_feed.py Show resolved Hide resolved
@Bartvaderkin
Copy link
Contributor Author

Bartvaderkin commented Jan 16, 2024

@stevenbal @pi-sigma I applied the PR fixes.

Also: there was a mistake in the logic where the incoming-notification handler would check and bail early for emailable Users and would never reach the userfeed hook if the users had a bad email or email-notifications disabled. I fixed it in the last commit.

@alextreme alextreme merged commit c77e5ff into develop Jan 16, 2024
14 checks passed
@alextreme alextreme deleted the feature/1760-userfeed branch January 16, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants