Bookwyrm groups #1490

hughrun · 2021-10-03T03:17:23Z

Wow, this turned out to be bigger than I expected. Apologies, if I make any in future I'll try to break the changes up a bit.

This PR relates to:

#1318
#170

This PR creates:

Groups that a user can join via a GroupMembership
A group curation option for Book Lists
A group value for lists, so that lists can be associated with a group for curation purposes

Groups do not currently interact with ActivityPub, but could. I am not confident to implement this functionality at this stage, hence not including it, but that's the only reason. I'd be happy to look into this for a later PR. Hopefully I haven't accidentally inluded any AP functionality where it shouldn't be.

Outstanding problems/questions to resolve/things to note

I can't work out how to adjust privacy_filter so that Group members can see groups they wouldn't otherwise see (i.e. groups with privacy levels of direct, or of followers when the viewer isn't a follower), whilst ensuring that the privacy filter nevertheless works in all other cases (e.g. when a user is blocked). Currently therefore privacy_filter is NOT applied to group list display on group pages (however raise_visible_to_user will still prevent anyone from seeing a list they shouldn't have access to)
I re-used some of the user suggestions code for new users, for searching for potential group members. I have, however, created a new filter function to exlude non-local users, for use until groups are AP compliant.
I am assuming that I should not commit anything in the migrations directory, and that maintainers will run makemigrations where appropriate: let me know if this assumption is wrong.
I am not sure what to do about group members who block each other. Currently:
1. If the group.user has blocked a user, the second user cannot access the group page but can access any lists assigned to the group if they are owned by someone else, and can add & delete items on those lists;
2. If a member of a group who is not group.user creates a list, other members who are blocked by that user cannot see the list (but can see it listed on the group page)
3. Users who are blocked cannot invite the blocker to join a group because they won't appear in the user-inviting search results.
The second case could be fixed if privacy_filter worked for groups and group lists. The first case may not matter much because a group owner can simply remove any member from the group, but it is a bit weird and not really ideal.
I get a Object of type LocalStream is not JSON serializable error when attempting to unblock a user in my development environment. I'm not sure whether this is an actual bug in Bookwyrm, a problem with my setup, or if I somehow broke something in the codebase. This may need to be checked!
I'm guessing you probably want some tests - I created some new classes and filters. What are the expectations/requirements around this?

Groups

any user may create a Group.
this group.user may send a GroupMemberInvitation to any other user as long as they are not blocked either way
the invitee must accept the GroupMemberInvitation to create a GroupMembership.
members (users with a GroupMembership connecting them to a Group) can:
1. create a List with a curation value of group and assign the List to this group
2. See all Lists associated with the group, on the group home page
3. see the list page for any List associated with the group, regardless of its privacy value (unless the list.user has blocked them)
4. add books to and remove books from any List associated with the group (unless the list.user has blocked them)
Groups have their own page visible to all users, at /group/{id}

Lists

Lists now have a new curation value of group
Group-curated lists must be associated with one (and only one) Group
Any member of the group can add and remove books to/from the list, unless blocked by the list.user.
Lists can have their curation value changed at any point: either by changing it to a non-group curation (closed, curated, open), or by changing the group it is associated with.
If a list's curation value is changed, group members no longer have access (unless curation is "public")
If a user is removed from the group, their group-curated lists will be changed to closed curation with a group of None.

Notifications

Group members are notified when:

a new member joins
a member leaves
a member is removed
a book is added to a group-curated list

group.users (group owners) are notified when:

a user accepts their invitation to join

Users are notified when:

a group.user invites them to join a group

Abuse and privacy

I have attempted to think about the potential for abuse and mitigate it, however I've probably missed something, and as noted above there are some outstanding problems to resolve.

Users must be invited to join a group: you cannot simply make someone a member of a group without them opting in
Group curated lists are removed from a group when the list.user leaves the group and given curation=private
Users can leave voluntarily or be removed by the group.owner
The same privacy levels apply to groups and group-curated lists as for any other object. This means if I group-curate a list with privacy level direct, only group members will be able to access it.
privacy_filter needs to be adjusted, or an alternative created for groups, so that the ability to see the existence of a list is also determined by privacy levels (e.g. direct lists don't appear on a group's list of Lists)

Design

As much as possible all changes are based on existing code and attempt to integrate seamlessly. I'm open to presenting any of the new options differently.

adds a group creation form to user dashboard

Whoops

- add list cards to groups page based on lists page - add sort to members on group page

you probably want this otherwise nothing previously added for group creation will work :-)

- creates groups/user_groups template for listing a user's groups on their user page

- search for users to add to a group - display suggested users on search results screen TODO: actaully enable users to be added! TODO: groups/suggested_users probably could be replaced with some logic in snippets/suggested_users.html

- this will also enable members to be removed easily by managers in a future commit.

- remove GroupList model - add a group foreign key value to List model - remove reference to lists in Group model

Allows 'group' to be blank when saving a list. Removes the 'group' field when saving a list with curation other than 'group' - this stops the list "sticking" to a group after it is changed from group curation to something else.

When creating or editing a list, the group selection dropdown will only appear if the user selects "group" as the curation option (or it is already selected). - fix typo in bookwyrm.js comments - add data-hides trigger for hiding elements after they have been unhidden, where simple toggles are not the right approach

This will allow privacy management to use existing code. Some template updates also are for rationalising how groups are created and edited.

hughrun · 2021-10-14T09:24:17Z

Thanks for these suggestions :-)

Re 1 and 2, this was definitely there at some point, so I must have accidentally broken both of those when I was refactoring. If there are no groups it should invite you to make a group with a link that takes you to your Groups page (which has the button to create a group)

Re 3 - makes sense, I think moving the searchbox into the main panel is the better solution.

Re 4 and 5 I agree but managing state is something I always find tricky so if it can be a follow up PR that would be preferable so I can look into how best to do it and any examples of ways you've handled anything similar.

Re 6 - absolutely!

Re 7 - sounds straightforward, will add it to the list of other fixes.

You should expect a (hopefully) final load of fixes this weekend!

- don't unnecessarily query DB in List views - use more efficient query in remove_from_group List class method

This reverts commit 41f27a4. I forgot that update() can only be done on a query result, not on an object, so we will need to go back to querying in order to update rather than saving.

Use .save() twice, but with broadcast=False on the second update. This is more efficient than doing a query and update() and avoids the duplicate AP broadcast.

- fix form details not appearing in group member search view - fix query term appearing in main search box when searching for new members - direct request user back to the group rather than the user when adding a user to a group

- require confirmation before removing a member - require confirmation before removing self - make button text less verbose - use more standardised formatting for group editing form - improve button colours - add missing trans tags - reload group page when removing member

Whoops, forgot to add this functionality earlier. - allow owner to delete a group - change all group lists to closed curation with group=False when group deleted

hughrun · 2021-10-16T07:08:59Z

@mouse-reeve LOL so clearly I need some help with tests, I've managed to break ALL of them.

Other than that:

all suggestions from your code review have been resolved
all suggestions in your comment above have been resolved
users can now delete their groups (whoops, we both missed that one) and any group lists are changed to closed curation with the group removed

Looking forward to seeing this in production once we've sorted out the tests.

mouse-reeve · 2021-10-16T14:19:45Z

All tests failing is so much better than just some tests failing. It looks like the migrations need to be updated - if you merge in main and then run ./bw-dev makemigrations --merge, that should create a new migration file that resolves the conflicts and you'll be good to go.

mouse-reeve

Once the tests are passing I think this is good to go :)

There are database migrations in main ahead of this branch so they need to be merged in to the branch before we can merge back into main.

bookwyrm/tests/models/test_group.py

bookwyrm/tests/views/test_group.py

marek-lach · 2021-10-17T20:50:11Z

Some fediverse applications implement the URL for a group as "https://tavern.town/`groups`/sneakerheads". Bookwyrm could probably accept both /group, and also /groups as a valid group URL, to be compatible in this regard with as many fediverse software as possible?

Awesome feature tho,

mouse-reeve · 2021-10-17T20:53:25Z

Thank you! Right now, groups aren't federated (see #1548). In terms of compatibility, the url shouldn't make a difference, since the applications should be providing those in the activitypub serializations

hughrun added 30 commits September 24, 2021 07:49

add views for groups

3e3c90e

add models for groups

b74cd37

update templates for groups

71b1c61

add group templates

99b5335

add groups urls

e07a25e

create group form

4e93b09

adds a group creation form to user dashboard

group creation form can now be submitted!

f32a2cc

Whoops

add group page

9b6d2a9

add group page template

264d9b2

add user cards to group pages

86a60d5

add list cards to groups page

d4fcf88

- add list cards to groups page based on lists page - add sort to members on group page

add create_group to __init__.py

273ad9a

you probably want this otherwise nothing previously added for group creation will work :-)

user groups listing template

8c326ec

- creates groups/user_groups template for listing a user's groups on their user page

find users for groups

cbe172d

- search for users to add to a group - display suggested users on search results screen TODO: actaully enable users to be added! TODO: groups/suggested_users probably could be replaced with some logic in snippets/suggested_users.html

update urls and group view for searching users to add to group

7c0deab

improve naming of templates and urls for groups

8d17f88

smaller cards for group members

e800106

- this will also enable members to be removed easily by managers in a future commit.

add and remove users from groups

b645d75

identify group manager

62a5ed7

better logic for identifying group manager

035fc52

don't allow non-manager to add and remove group members

ec07205

update group and list models

6861984

- remove GroupList model - add a group foreign key value to List model - remove reference to lists in Group model

add group field to ListForm

b921d66

pass group value to list views and vice-versa

f3a3ba5

create group curated lists

8bfc71d

remove list from group when changing curation

5fccb99

Allows 'group' to be blank when saving a list. Removes the 'group' field when saving a list with curation other than 'group' - this stops the list "sticking" to a group after it is changed from group curation to something else.

refer to group in group lists created_text

0e2095b

rationalise group creation and prep for group privacy

2874e52

change group owner from 'manager' to 'user'

f318169

This will allow privacy management to use existing code. Some template updates also are for rationalising how groups are created and edited.

hughrun added 15 commits October 15, 2021 17:56

clean up List db queries

41f27a4

- don't unnecessarily query DB in List views - use more efficient query in remove_from_group List class method

remove useless field in Notifications model

1634c87

Revert "clean up List db queries"

2f38af4

This reverts commit 41f27a4. I forgot that update() can only be done on a query result, not on an object, so we will need to go back to querying in order to update rather than saving.

efficiency improvement to remove_from_group

97ea61d

clean up templates

864d8ce

fix local filter for suggested users

e261bfd

formatting fix

6f13c0d

improve group removal flow

afd00cc

Use .save() twice, but with broadcast=False on the second update. This is more efficient than doing a query and update() and avoids the duplicate AP broadcast.

group view fixes

91534af

- fix form details not appearing in group member search view - fix query term appearing in main search box when searching for new members - direct request user back to the group rather than the user when adding a user to a group

(hopefully) fix group tests

1736419

delete groups

2a31bf2

Whoops, forgot to add this functionality earlier. - allow owner to delete a group - change all group lists to closed curation with group=False when group deleted

database migrations for groups

a476b52

code formatting

0cd8ece

code indenting fix

8192d6a

hughrun requested a review from mouse-reeve October 16, 2021 07:10

mouse-reeve approved these changes Oct 16, 2021

View reviewed changes

hughrun added 2 commits October 17, 2021 06:22

Merge branch 'main' into bookwyrm-groups

6e5c0cc

There are database migrations in main ahead of this branch so they need to be merged in to the branch before we can merge back into main.

add makemigrations merge file

9116af1

mouse-reeve reviewed Oct 16, 2021

View reviewed changes

bookwyrm/tests/models/test_group.py Outdated Show resolved Hide resolved

bookwyrm/tests/views/test_group.py Outdated Show resolved Hide resolved

adjust Group test mocks

ca47f83

hughrun mentioned this pull request Oct 17, 2021

Groups should be Activity Pub actors and allow remote members #1548

Open

mouse-reeve merged commit a27a55b into bookwyrm-social:main Oct 17, 2021

hughrun deleted the bookwyrm-groups branch October 18, 2021 02:17

mouse-reeve mentioned this pull request Oct 20, 2021

List curation option - "Limited"/"Group" #1318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bookwyrm groups #1490

Bookwyrm groups #1490

hughrun commented Oct 3, 2021

hughrun commented Oct 14, 2021

hughrun commented Oct 16, 2021

mouse-reeve commented Oct 16, 2021

mouse-reeve left a comment

marek-lach commented Oct 17, 2021

mouse-reeve commented Oct 17, 2021

Bookwyrm groups #1490

Bookwyrm groups #1490

Conversation

hughrun commented Oct 3, 2021

Outstanding problems/questions to resolve/things to note

Groups

Lists

Notifications

Abuse and privacy

Design

hughrun commented Oct 14, 2021

hughrun commented Oct 16, 2021

mouse-reeve commented Oct 16, 2021

mouse-reeve left a comment

Choose a reason for hiding this comment

marek-lach commented Oct 17, 2021

mouse-reeve commented Oct 17, 2021