-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Lists #5703
Lists #5703
Conversation
92d2690
to
305ee4c
Compare
f953418
to
983a648
Compare
983a648
to
31bceab
Compare
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.
needs an update to remove_status_service_spec
for Feed -> HomeFeed.
needs a corresponding /documentation PR
render_views | ||
|
||
let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) } | ||
let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'follow') } |
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.
wrong scopes
def initialize(list) | ||
@type = :list | ||
@id = list.id | ||
end |
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.
No from_database
for lists?
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.
Same question from me.
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.
I'm fine punting on this for now.
app/policies/list_policy.rb
Outdated
@@ -0,0 +1,13 @@ | |||
# frozen_string_literal: true | |||
|
|||
class ListPolicy < ApplicationPolicy |
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.
Is this ever used by Doorkeeper? I couldn't find any indication that doorkeeper checked pundit policies....
app/lib/feed_manager.rb
Outdated
@@ -27,33 +27,41 @@ def filter?(timeline_type, status, receiver_id) | |||
end | |||
|
|||
def push(timeline_type, account, status) |
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.
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)
Any thought of exposing these as AS2 Collections? |
- Creating and modifying lists merely requires "write" scope - Fetching information about lists merely requires "read" scope
a01d3fb
to
95a4dfa
Compare
@cwebber right now lists are entirely private and no one else can see them. If we add other functionality then we might consider that |
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.
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.
@@ -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) |
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.
What if you're in one of your own lists?
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.
You can't follow yourself so you can't add yourself to a list.
|
||
PushUpdateWorker.perform_async(account.id, status.id) if push_update_required?(timeline_type, account.id) | ||
|
||
def push_to_home(account, status) |
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.
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.)
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.
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
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.
My thought was that it mirrors push_to_hashtag, which isn't in feed_manager but does need to happen
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.
Huh? No, hashtags timelines are not stored in redis
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.
Yes, but it mirrors the necessity of calling all of those different functions (see the calling site)
allow(controller).to receive(:doorkeeper_token) { token } | ||
end | ||
|
||
context 'with a user context' do |
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.
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
Outdated
require 'rails_helper' | ||
|
||
RSpec.describe ListAccount, type: :model do | ||
pending "add some examples to (or delete) #{__FILE__}" |
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.
I'd either add tests or drop this (and list_spec.rb
).
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.
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
@@ -3,7 +3,7 @@ | |||
# | |||
# Table name: accounts | |||
# | |||
# id :bigint not null, primary key | |||
# id :integer not null, primary key | |||
# username :string default(""), not null |
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.
Huh? Why would we go back to integer?
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.
These model annotations are auto-generated. I've deleted the rest of your comments because they're not a concern.
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.
Right. Still, noisy for no reason, and I don't understand why they would be generated that way? db/schema.rb
seems fine, though.
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.
see the conversation on #5461
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.
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.
def initialize(list) | ||
@type = :list | ||
@id = list.id | ||
end |
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.
Same question from me.
Still needs |
Merge upstream (mastodon#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
@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! |
Fix #134
Web UI implementation should come in a different PR.
New APIs:
GET /api/v1/timelines/list/:id
Timeline of a listGET /api/v1/lists
All your listsPOST /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 listPOST/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 timelineOther information: