/mastodon

Lists #5703

Merged
merged 9 commits into from Nov 17, 2017

Conversation

Reviewers

@aschmitz aschmitz

@ThibG ThibG

@nightpool nightpool

Assignees
No one assigned
Labels
api
Projects
None yet
Milestone
No milestone
7 participants
@Gargron @cwebber @nightpool @aquariusdev @Cassolotl @aschmitz @ThibG
@Gargron
Member

Gargron commented Nov 15, 2017 •
edited
Edited 8 times
  • @Gargron Gargron edited Nov 16, 2017 (most recent)
  • @Gargron Gargron edited Nov 16, 2017
  • @Gargron Gargron edited Nov 16, 2017
  • @Gargron Gargron edited Nov 16, 2017
  • @Gargron Gargron edited Nov 16, 2017
  • @Gargron Gargron edited Nov 16, 2017
  • @Gargron Gargron edited Nov 16, 2017
  • @Gargron Gargron edited Nov 16, 2017

Fix #134

  • Database structure for storing lists
  • Adjust FeedManager to handle list timelines
  • Add lists to streaming API
  • API for managing lists

Web UI implementation should come in a different PR.

New APIs:

  • GET /api/v1/timelines/list/:id Timeline of a list
  • GET /api/v1/lists All your lists
  • POST /api/v1/lists Create a new list (allowed param: title)
  • GET/PUT/DELETE /api/v1/lists/:id
  • GET /api/v1/lists/:id/accounts All accounts in the list
  • POST/DELETE /api/v1/lists/:id/accounts Add or remove accounts to/from the list (array param: account_ids)

New streaming API:

  • /api/v1/streaming/list?list=:id Subscribe to list timeline

Other information:

  • Lists are private to their creator
  • Only people you follow can be added to your lists
  • If a follow stops existing, their list membership also stops existing

@Gargron Gargron added api work in progress labels Nov 15, 2017

@Gargron Gargron removed the work in progress label Nov 16, 2017

@nightpool

nightpool requested changes Nov 16, 2017
View changes

needs an update to remove_status_service_spec for Feed -> HomeFeed.

needs a corresponding /documentation PR

spec/controllers/api/v1/lists/accounts_controller_spec.rb
+ render_views
+
+ let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) }
+ let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'follow') }

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 16, 2017

Collaborator

wrong scopes
@nightpool

nightpool Nov 16, 2017

Collaborator

wrong scopes
app/models/list_feed.rb
+ def initialize(list)
+ @type = :list
+ @id = list.id
+ end

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 16, 2017

Collaborator

No from_database for lists?
@nightpool

nightpool Nov 16, 2017

Collaborator

No from_database for lists?

This comment has been minimized.

Show comment
Hide comment
@ThibG

ThibG Nov 17, 2017

Collaborator

Same question from me.
@ThibG

ThibG Nov 17, 2017

Collaborator

Same question from me.

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 17, 2017

Collaborator

I'm fine punting on this for now.
@nightpool

nightpool Nov 17, 2017

Collaborator

I'm fine punting on this for now.
app/policies/list_policy.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class ListPolicy < ApplicationPolicy

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 16, 2017

Collaborator

Is this ever used by Doorkeeper? I couldn't find any indication that doorkeeper checked pundit policies....
@nightpool

nightpool Nov 16, 2017

Collaborator

Is this ever used by Doorkeeper? I couldn't find any indication that doorkeeper checked pundit policies....
app/lib/feed_manager.rb
@@ -27,33 +27,41 @@ def filter?(timeline_type, status, receiver_id)
end
def push(timeline_type, account, status)

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 16, 2017

Collaborator

Considering that we only ever use push(:home) (as far as I can tell....) and we're adding a push_to_list anyway, we should probably make a push_to_home method, and then either get rid of push or make it private and get rid of any code duplication between it and push_to_list

(same with unpush)
@nightpool

nightpool Nov 16, 2017 •
edited
Edited 1 time
  • @nightpool nightpool edited Nov 16, 2017 (most recent)

Collaborator

Considering that we only ever use push(:home) (as far as I can tell....) and we're adding a push_to_list anyway, we should probably make a push_to_home method, and then either get rid of push or make it private and get rid of any code duplication between it and push_to_list

(same with unpush)

@nightpool nightpool requested review from nightpool and removed request for nightpool Nov 16, 2017

@cwebber

This comment has been minimized.

Show comment
Hide comment
@cwebber

cwebber Nov 16, 2017

Any thought of exposing these as AS2 Collections?

cwebber commented Nov 16, 2017

Any thought of exposing these as AS2 Collections?
@Gargron
Adjust scopes for new APIs 
- Creating and modifying lists merely requires "write" scope
- Fetching information about lists merely requires "read" scope
Loading status checks…
95a4dfa
@nightpool

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 17, 2017

Collaborator

@cwebber right now lists are entirely private and no one else can see them. If we add other functionality then we might consider that
Collaborator

nightpool commented Nov 17, 2017

@cwebber right now lists are entirely private and no one else can see them. If we add other functionality then we might consider that
@aschmitz

aschmitz reviewed Nov 17, 2017
View changes

This looks reasonably good overall. The bulk endpoint is apparently new, so it would be good to make sure it's appropriately documented. A few minor comments for possible changes, but I don't know that any of them are really necessary.

app/services/fan_out_on_write_service.rb
@@ -30,15 +31,25 @@ def call(status)
def deliver_to_self(status)
Rails.logger.debug "Delivering status #{status.id} to author"
- FeedManager.instance.push(:home, status.account, status)
+ FeedManager.instance.push_to_home(status.account, status)

This comment has been minimized.

Show comment
Hide comment
@aschmitz

aschmitz Nov 17, 2017

Contributor

What if you're in one of your own lists?
@aschmitz

aschmitz Nov 17, 2017

Contributor

What if you're in one of your own lists?

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Nov 17, 2017

Member

You can't follow yourself so you can't add yourself to a list.
@Gargron

Gargron Nov 17, 2017

Member

You can't follow yourself so you can't add yourself to a list.
app/lib/feed_manager.rb
-
- PushUpdateWorker.perform_async(account.id, status.id) if push_update_required?(timeline_type, account.id)
-
+ def push_to_home(account, status)

This comment has been minimized.

Show comment
Hide comment
@aschmitz

aschmitz Nov 17, 2017

Contributor

Is there a good reason to call push_to_home and push_to_list separately outside of FeedManager? Should we just expose push_to_user and let FeedManager sort out lists? (This seems less efficient, but perhaps cleaner. I can see arguments on either side.)
@aschmitz

aschmitz Nov 17, 2017

Contributor

Is there a good reason to call push_to_home and push_to_list separately outside of FeedManager? Should we just expose push_to_user and let FeedManager sort out lists? (This seems less efficient, but perhaps cleaner. I can see arguments on either side.)

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Nov 17, 2017

Member

As I started writing the code, the push behaviour was different enough that I had to create a separate push_to_list method. But as I refactored, they turned out pretty similar after all. However, I am absolutely OK with keeping them separate for now. As they say, the DRY principle kicks in for n >= 3
@Gargron

Gargron Nov 17, 2017

Member

As I started writing the code, the push behaviour was different enough that I had to create a separate push_to_list method. But as I refactored, they turned out pretty similar after all. However, I am absolutely OK with keeping them separate for now. As they say, the DRY principle kicks in for n >= 3

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 17, 2017

Collaborator

My thought was that it mirrors push_to_hashtag, which isn't in feed_manager but does need to happen
@nightpool

nightpool Nov 17, 2017

Collaborator

My thought was that it mirrors push_to_hashtag, which isn't in feed_manager but does need to happen

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Nov 17, 2017

Member

Huh? No, hashtags timelines are not stored in redis
@Gargron

Gargron Nov 17, 2017

Member

Huh? No, hashtags timelines are not stored in redis

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 17, 2017

Collaborator

Yes, but it mirrors the necessity of calling all of those different functions (see the calling site)
@nightpool

nightpool Nov 17, 2017

Collaborator

Yes, but it mirrors the necessity of calling all of those different functions (see the calling site)
spec/controllers/api/v1/timelines/list_controller_spec.rb
+ allow(controller).to receive(:doorkeeper_token) { token }
+ end
+
+ context 'with a user context' do

This comment has been minimized.

Show comment
Hide comment
@aschmitz

aschmitz Nov 17, 2017

Contributor

It may be worth testing with a different user's context as well, to ensure the list isn't accidentally exposed.
@aschmitz

aschmitz Nov 17, 2017

Contributor

It may be worth testing with a different user's context as well, to ensure the list isn't accidentally exposed.
spec/models/list_account_spec.rb
+require 'rails_helper'
+
+RSpec.describe ListAccount, type: :model do
+ pending "add some examples to (or delete) #{__FILE__}"

This comment has been minimized.

Show comment
Hide comment
@aschmitz

aschmitz Nov 17, 2017

Contributor

I'd either add tests or drop this (and list_spec.rb).
@aschmitz

aschmitz Nov 17, 2017

Contributor

I'd either add tests or drop this (and list_spec.rb).

@ThibG ThibG self-requested a review Nov 17, 2017

@ThibG

ThibG reviewed Nov 17, 2017
View changes

I'm confused about the bigint VS integer things and I wonder about not having a method to load lists from database when Redis cache doesn't exist

app/models/account.rb
@@ -3,7 +3,7 @@
#
# Table name: accounts
#
-# id :bigint not null, primary key
+# id :integer not null, primary key
# username :string default(""), not null

This comment has been minimized.

Show comment
Hide comment
@ThibG

ThibG Nov 17, 2017

Collaborator

Huh? Why would we go back to integer?
@ThibG

ThibG Nov 17, 2017

Collaborator

Huh? Why would we go back to integer?

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 17, 2017

Collaborator

These model annotations are auto-generated. I've deleted the rest of your comments because they're not a concern.
@nightpool

nightpool Nov 17, 2017

Collaborator

These model annotations are auto-generated. I've deleted the rest of your comments because they're not a concern.

This comment has been minimized.

Show comment
Hide comment
@ThibG

ThibG Nov 17, 2017

Collaborator

Right. Still, noisy for no reason, and I don't understand why they would be generated that way? db/schema.rb seems fine, though.
@ThibG

ThibG Nov 17, 2017

Collaborator

Right. Still, noisy for no reason, and I don't understand why they would be generated that way? db/schema.rb seems fine, though.

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 17, 2017

Collaborator

see the conversation on #5461
@nightpool

nightpool Nov 17, 2017

Collaborator

see the conversation on #5461

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Nov 17, 2017

Member

Every db:migrate is gonna reset these to this. So I'd rather have them wrong than deal with noise in every PR. I shouldn't have merged #5461 but I did not realize annotate would not pick up the correct types, thought something just got forgotten because of the unusual way we performed the snowflake migrations.
@Gargron

Gargron Nov 17, 2017

Member

Every db:migrate is gonna reset these to this. So I'd rather have them wrong than deal with noise in every PR. I shouldn't have merged #5461 but I did not realize annotate would not pick up the correct types, thought something just got forgotten because of the unusual way we performed the snowflake migrations.
app/models/list_feed.rb
+ def initialize(list)
+ @type = :list
+ @id = list.id
+ end

This comment has been minimized.

Show comment
Hide comment
@ThibG

ThibG Nov 17, 2017

Collaborator

Same question from me.
@ThibG

ThibG Nov 17, 2017

Collaborator

Same question from me.

@tootsuite tootsuite deleted a comment from ThibG Nov 17, 2017

@tootsuite tootsuite deleted a comment from ThibG Nov 17, 2017

@tootsuite tootsuite deleted a comment from ThibG Nov 17, 2017

@nightpool

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Nov 17, 2017

Collaborator

Still needs remove_status_service_spec update.
Collaborator

nightpool commented Nov 17, 2017

Still needs remove_status_service_spec update.
@Gargron
Clean up tests
Loading status checks…
93857ff

@Gargron Gargron merged commit 24cafd7 into master Nov 17, 2017
3 checks passed

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Gargron Gargron deleted the feature-lists branch Nov 17, 2017

yipdw added a commit to glitch-soc/mastodon that referenced this pull request Nov 17, 2017

@yipdw
Merge pull request #224 from yipdw/merge-upstream 
Merge upstream (tootsuite/mastodon#5703)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
284e2cd

@rinsuki rinsuki referenced this pull request in cinderella-project/iMast Nov 22, 2017

Merged

Support Lists #72

@Gargron Gargron referenced this pull request Nov 27, 2017

Merged

Bump version to 2.1.0rc1 #5834

10 of 10 tasks complete

@remram44 remram44 referenced this pull request Dec 4, 2017

Closed

View content from a list of users #952

This was referenced Dec 5, 2017

Merged
Merged

cobodo pushed a commit to cobodo/mastodon that referenced this pull request Dec 6, 2017

@Gargron cobodo
Lists (#5703) 
* Add structure for lists

* Add list timeline streaming API

* Add list APIs, bind list-account relation to follow relation

* Add API for adding/removing accounts from lists

* Add pagination to lists API

* Add pagination to list accounts API

* Adjust scopes for new APIs

- Creating and modifying lists merely requires "write" scope
- Fetching information about lists merely requires "read" scope

* Add test for wrong user context on list timeline

* Clean up tests
0791334

@aquariusdev aquariusdev referenced this pull request in glitch-soc/mastodon Apr 18, 2018

Open

feature outline: Lists #12

@aquariusdev

This comment has been minimized.

Show comment
Hide comment
@aquariusdev

aquariusdev Apr 18, 2018

Can you add the ability to add/remove users to/from lists on their profile?

Mockup 1, borrowed a bit from Diaspora:
mastodon aspects dropdown mockup

Mockup 2, using existing menus on Mastodon:
mastodon add to remove from lists thru profile

Mockup 3, using Twitter's Add to/Remove from Lists pop-up:
twitter multi-select

aquariusdev commented Apr 18, 2018 •
edited
Edited 1 time
  • @aquariusdev aquariusdev edited Apr 18, 2018 (most recent)

Can you add the ability to add/remove users to/from lists on their profile?

Mockup 1, borrowed a bit from Diaspora:
mastodon aspects dropdown mockup

Mockup 2, using existing menus on Mastodon:
mastodon add to remove from lists thru profile

Mockup 3, using Twitter's Add to/Remove from Lists pop-up:
twitter multi-select
@Cassolotl

This comment has been minimized.

Show comment
Hide comment
@Cassolotl

Cassolotl Apr 18, 2018

@aquariusdev Since this is a pull request that was merged a while ago, it's probably best to create a new issue for your feature request!

Cassolotl commented Apr 18, 2018 •
edited
Edited 2 times
  • @Cassolotl Cassolotl edited Apr 18, 2018 (most recent)
  • @Cassolotl Cassolotl edited Apr 18, 2018

@aquariusdev Since this is a pull request that was merged a while ago, it's probably best to create a new issue for your feature request!

@aquariusdev aquariusdev referenced this pull request Apr 18, 2018

Open

"Add or remove from lists" option + multi-choice list selection on profile #7187

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment