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

Add 'Stopped Reading' shelf #1934

Merged

Conversation

tversteeg
Copy link
Contributor

@tversteeg tversteeg commented Feb 12, 2022

Fixes #1385.

When the shelf is read the label is 'Finished', otherwise it's 'Until'.
@mouse-reeve
Copy link
Member

Wow thank you! I just approved the automated checks, which will let you know if there are are syntax tweaks or tests failing. If you have any issue resolving them, I'm super happy to help -- running ./bw-dev black fixes most python issues.

Once I get a chance, I'll check out the code, test it locally, and let you know if anything needs to be tweaked.

Just from the outset, how do you feel about using "stopped reading" rather than "partially read"? To me, "partially read" sounds really similar to "currently reading", whereas "stopped reading" differentiates them.

@tversteeg tversteeg changed the title Add 'Partially Read' shelf Add 'Stopped Reading' shelf Feb 12, 2022
@tversteeg tversteeg force-pushed the partially-read-shelf branch from f072dff to c88b348 Compare February 12, 2022 18:49
@tversteeg
Copy link
Contributor Author

It seems the black I run in ./bw-dev black is different from the one in the CI.

Also, when I run ./bw-dev pytest I don't get any meaningful output:

+ case "$CMD" in
+ execweb pytest --no-cov-on-fail
+ docker-compose exec web pytest --no-cov-on-fail```

@mouse-reeve
Copy link
Member

Sorry about that -- #1941 will fix that but I have to get the dang css linter working first. To run the tests, use docker-compose run --rm web pytest --no-cov-on-fail. For black, the version in requirements.txt is correct, but likely the version in your dev environment is out of date. Try docker-compose run --rm web pip install --upgrade black (or just revert the file that's causing the problem: git checkout main bookwyrm/templatetags/shelf_tags.py)

@mouse-reeve
Copy link
Member

You can fix the tests all failing by creating a merge migration using ./bw-dev makemigrations --merge.

There are a few things missing that I saw at a quick test:

  1. There's a secret template mode for the reading status models that is used when javascript is unavailable, they just display the modals on their own page. The stopped reading modal needs an analagous modal to the ones that are already there: https://github.com/bookwyrm-social/bookwyrm/blob/main/bookwyrm/templates/reading_progress/finish.html. If you want to see those in action, you can disable javascript in your browser and click the buttons that open the modals.
  2. I'm seeing the shelf change working great, but I have't gotten it to produce a status.
  3. The stopped shelf isn't being added for existing users -- you can add a python function to the migration that updates existing entries in the database.

Let me know if I can help or you would like me to take on any parts of it. I really appreciate your work on this!

@tversteeg
Copy link
Contributor Author

Let me know if I can help or you would like me to take on any parts of it. I really appreciate your work on this!

Thanks for the suggestions! I'm on holiday now for a week without access to a computer so I can't fix these issues soon, if you think this feature is important you can pick it up, otherwise I can implement your suggestions later. Both are fine with me!

@tversteeg tversteeg force-pushed the partially-read-shelf branch from 0a03d73 to 8deee22 Compare February 25, 2022 21:39
@tversteeg
Copy link
Contributor Author

I've fixed the modal not working without javascript (I love it that you support don't make javascript mandatory, btw).

I'm still working on the status update, which is a bit more work than anticipated, I won't be able to finish that today.

For adding the shelf to existing users using python in the migration, could you help me with that? This is a bit outside my comfort zone, and I'm scared of ruining the database 😄

@mouse-reeve
Copy link
Member

For sure! I'll do that today. Thank you :)

@mouse-reeve
Copy link
Member

mouse-reeve commented Feb 26, 2022

I wanted to add that as a patch pr so I could test it but github glitched out so I had to commit directly. Oh well! I will fix the CI problems I have created 😄

@tversteeg
Copy link
Contributor Author

That's fine, I feel like this whole PR should be squashed anyway :)
I've got a proper status to show up on my side. I think everything is working fine now but I might miss something..
Thanks for the database script by the way!

@mouse-reeve
Copy link
Member

I'm ready to merge this -- I'm going to merge it into a local branch so I can fix a couple things I janked up when I added to the migration, and then merge from that branch to main. Thanks for your work and for being patient with the delay on reviewing this!

@mouse-reeve mouse-reeve changed the base branch from main to stopped-shelf March 16, 2022 20:50
@mouse-reeve mouse-reeve merged commit f2b0b30 into bookwyrm-social:stopped-shelf Mar 16, 2022
@tversteeg
Copy link
Contributor Author

No problem, thanks for helping me with this PR and the awesome software!

@mouse-reeve mouse-reeve mentioned this pull request Mar 26, 2022
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.

Better support for abandoning a book
2 participants