hovel / pybbm

Django forum solution. Tested, documented, shipped with example project.
BSD 2-Clause "Simplified" License
225 stars 151 forks source link

Move 'mark_read' call out of 'get_context_data' and into 'get'. #197

Closed skolsuper closed 7 years ago

skolsuper commented 9 years ago

Two reasons for moving the method call:

  1. I wouldn't expect 'get_context_data' to have side effects.
  2. It ensures that the view returns without error before marking the topic read

This PR also modifies an inefficient query in mark_read that checks for unread topics in forum.

DylannCordel commented 7 years ago

LGTM but If I may bring my 2cts:

DylannCordel commented 7 years ago

Forget the third point : I'm working on a PR to remove those @skipUnlessDBFeature. There is still a test which always fails with MySQL, even with sleep calls. I think there is a bug in pybbm read tracking system with MySQL. If you want to have a look on the WIP : webu/pybbm@ec7d7d808be5237b6d550e5c01ebba5274d50194

DylannCordel commented 7 years ago

@skolsuper : see #258 for the third point.

skolsuper commented 7 years ago

Agree with points 1 and 2 but don't have time to fix. I have given push access to my fork so you can touch up this branch if you think it's worthwhile.

DylannCordel commented 7 years ago

@skolsuper I rebase the PR and add points 1 and 2. Thanks for the access.

lampslave commented 7 years ago

Better late than never :) Thank you.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 25b761ca6f3706f786631457cb8200a5124b4f31 on skolsuper:mark_read into on hovel:master.