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
Keep track of progress through books over time #354
Changes from 35 commits
3beebe4
7ffc311
13229ea
00b8608
f524f0c
c9b2b4e
ff7d87b
a579ea5
f57d9ee
e7c0368
090cf2a
64fb88c
692aa08
97e49c4
85026b8
3b0b8f1
5f2ac6d
3cb2827
f86140c
6455cc7
9ed7d23
500f052
a951f20
7fadbee
4814788
1e13997
da8d8cd
2d15713
a4796cf
6e05dfd
ef05ac1
a4519d5
49893f4
0af4863
1ea8ea0
29140be
79e284e
60b4282
32346cf
edba55f
070fa04
85edee4
57607c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,4 +65,4 @@ jobs: | |
EMAIL_HOST_PASSWORD: "" | ||
EMAIL_USE_TLS: true | ||
run: | | ||
python manage.py test | ||
python manage.py test -v 3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# Generated by Django 3.0.7 on 2020-11-17 07:36 | ||
|
||
from django.conf import settings | ||
from django.db import migrations, models | ||
import django.db.models.deletion | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('bookwyrm', '0011_auto_20201113_1727'), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name='ProgressUpdate', | ||
fields=[ | ||
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('created_date', models.DateTimeField(auto_now_add=True)), | ||
('updated_date', models.DateTimeField(auto_now=True)), | ||
('remote_id', models.CharField(max_length=255, null=True)), | ||
('progress', models.IntegerField()), | ||
('mode', models.CharField(choices=[('PG', 'page'), ('PCT', 'percent')], default='PG', max_length=3)), | ||
('readthrough', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='bookwyrm.ReadThrough')), | ||
('user', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to=settings.AUTH_USER_MODEL)), | ||
], | ||
options={ | ||
'abstract': False, | ||
}, | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# Generated by Django 3.0.7 on 2020-11-28 00:07 | ||
|
||
from django.db import migrations | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('bookwyrm', '0013_book_origin_id'), | ||
('bookwyrm', '0012_progressupdate'), | ||
] | ||
|
||
operations = [ | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Generated by Django 3.0.7 on 2020-11-28 07:34 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('bookwyrm', '0014_merge_20201128_0007'), | ||
] | ||
|
||
operations = [ | ||
migrations.RenameField( | ||
model_name='readthrough', | ||
old_name='pages_read', | ||
new_name='progress', | ||
), | ||
migrations.AddField( | ||
model_name='readthrough', | ||
name='progress_mode', | ||
field=models.CharField(choices=[('PG', 'page'), ('PCT', 'percent')], default='PG', max_length=3), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# Generated by Django 3.0.7 on 2021-01-17 21:06 | ||
|
||
from django.db import migrations | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('bookwyrm', '0015_auto_20201128_0734'), | ||
('bookwyrm', '0036_annualgoal'), | ||
] | ||
|
||
operations = [ | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,21 @@ | |
|
||
from .base_model import BookWyrmModel | ||
|
||
class ProgressMode(models.TextChoices): | ||
PAGE = 'PG', 'page' | ||
PERCENT = 'PCT', 'percent' | ||
|
||
class ReadThrough(BookWyrmModel): | ||
''' Store progress through a book in the database. ''' | ||
''' Store a read through a book in the database. ''' | ||
user = models.ForeignKey('User', on_delete=models.PROTECT) | ||
book = models.ForeignKey('Edition', on_delete=models.PROTECT) | ||
pages_read = models.IntegerField( | ||
progress = models.IntegerField( | ||
null=True, | ||
blank=True) | ||
progress_mode = models.CharField( | ||
max_length=3, | ||
choices=ProgressMode.choices, | ||
default=ProgressMode.PAGE) | ||
start_date = models.DateTimeField( | ||
blank=True, | ||
null=True) | ||
|
@@ -24,3 +31,26 @@ def save(self, *args, **kwargs): | |
self.user.last_active_date = timezone.now() | ||
self.user.save() | ||
super().save(*args, **kwargs) | ||
|
||
def create_update(self): | ||
if self.progress: | ||
return self.progressupdate_set.create( | ||
user=self.user, | ||
progress=self.progress, | ||
mode=self.progress_mode) | ||
|
||
class ProgressUpdate(BookWyrmModel): | ||
''' Store progress through a book in the database. ''' | ||
user = models.ForeignKey('User', on_delete=models.PROTECT) | ||
readthrough = models.ForeignKey('ReadThrough', on_delete=models.PROTECT) | ||
progress = models.IntegerField() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should have a min value validator to prevent negative values here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added validators here and on ReadThrough.progress (and tests to make sure they work), but didn't actually use them anywhere. I'm not sure they'll have any effect on ProgressUpdate, but might on the ReadThrough depending on how we use them? In any case, having the validators is better than not |
||
mode = models.CharField( | ||
max_length=3, | ||
choices=ProgressMode.choices, | ||
default=ProgressMode.PAGE) | ||
|
||
def save(self, *args, **kwargs): | ||
''' update user active time ''' | ||
self.user.last_active_date = timezone.now() | ||
self.user.save() | ||
super().save(*args, **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<form class="field is-grouped is-small pl-2" action="/edit-readthrough" method="POST"> | ||
{% csrf_token %} | ||
<input type="hidden" name="id" value="{{ readthrough.id }}"/> | ||
<div class="control"> | ||
{% if readthrough.progress_mode == 'PG' %} | ||
on page | ||
{% else %} | ||
currently at | ||
{% endif %} | ||
</div> | ||
<div class="control"> | ||
<input | ||
aria-label="{% if readthrough.progress_mode == 'PG' %}Current page{% else %}Percent read{% endif %}" | ||
class="input is-small" type="text" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this type should be |
||
name="progress" size="3" value="{{ readthrough.progress|default:'' }}"> | ||
</div> | ||
<div class="control"> | ||
{% if readthrough.progress_mode == 'PG' %} | ||
{% if book.pages %} of {{ book.pages }} {% endif %} | ||
{% else %} % {% endif %} | ||
</div> | ||
<div class="control"> | ||
<button class="button is-small px-2" type="submit">Go</button> | ||
</div> | ||
</form> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from . import * |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
from django.test import TestCase, Client | ||
from django.utils import timezone | ||
from datetime import datetime | ||
|
||
from bookwyrm import models | ||
|
||
class ReadThrough(TestCase): | ||
def setUp(self): | ||
self.client = Client() | ||
|
||
self.work = models.Work.objects.create( | ||
title='Example Work' | ||
) | ||
|
||
self.edition = models.Edition.objects.create( | ||
title='Example Edition', | ||
parent_work=self.work | ||
) | ||
self.work.default_edition = self.edition | ||
self.work.save() | ||
|
||
self.user = models.User.objects.create_user( | ||
'cinco', 'cinco@example.com', 'seissiete', | ||
local=True, localname='cinco') | ||
|
||
self.client.force_login(self.user) | ||
|
||
def test_create_basic_readthrough(self): | ||
"""A basic readthrough doesn't create a progress update""" | ||
self.assertEqual(self.edition.readthrough_set.count(), 0) | ||
|
||
self.client.post('/start-reading/{}'.format(self.edition.id), { | ||
'start_date': '2020-11-27', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this might have the side effect of creating a "started reading" status and then triggering a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh rad, that's very helpful, thanks! I'll take a peek at that, hopefully the logs will confirm once they time out again in a couple hours 🙃 |
||
}) | ||
|
||
readthroughs = self.edition.readthrough_set.all() | ||
self.assertEqual(len(readthroughs), 1) | ||
self.assertEqual(readthroughs[0].progressupdate_set.count(), 0) | ||
self.assertEqual(readthroughs[0].start_date, | ||
datetime(2020, 11, 27, tzinfo=timezone.utc)) | ||
self.assertEqual(readthroughs[0].progress, None) | ||
self.assertEqual(readthroughs[0].finish_date, None) | ||
|
||
def test_create_progress_readthrough(self): | ||
self.assertEqual(self.edition.readthrough_set.count(), 0) | ||
|
||
self.client.post('/start-reading/{}'.format(self.edition.id), { | ||
'start_date': '2020-11-27', | ||
'progress': 50, | ||
}) | ||
|
||
readthroughs = self.edition.readthrough_set.all() | ||
self.assertEqual(len(readthroughs), 1) | ||
self.assertEqual(readthroughs[0].start_date, | ||
datetime(2020, 11, 27, tzinfo=timezone.utc)) | ||
self.assertEqual(readthroughs[0].progress, 50) | ||
self.assertEqual(readthroughs[0].finish_date, None) | ||
|
||
progress_updates = readthroughs[0].progressupdate_set.all() | ||
self.assertEqual(len(progress_updates), 1) | ||
self.assertEqual(progress_updates[0].mode, models.ProgressMode.PAGE) | ||
self.assertEqual(progress_updates[0].progress, 50) | ||
|
||
# Update progress | ||
self.client.post('/edit-readthrough', { | ||
'id': readthroughs[0].id, | ||
'progress': 100, | ||
}) | ||
|
||
progress_updates = readthroughs[0].progressupdate_set\ | ||
.order_by('updated_date').all() | ||
self.assertEqual(len(progress_updates), 2) | ||
self.assertEqual(progress_updates[1].mode, models.ProgressMode.PAGE) | ||
self.assertEqual(progress_updates[1].progress, 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I try to delete a readthrough that has an associated progress update, I get an error because of the protected relationship. I think this is a case where CASCADE would be better than PROTECT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, if a user is deleting a readthrough there's no reason to keep the progress updates. There's already a confirmation dialog for readthroughs, I bet I can add a "...and its X progress updates" to that just to make it clear.