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

Improve support for aspects/circles #8950

Merged
merged 9 commits into from Oct 17, 2018

Conversation

@Gargron
Copy link
Member

commented Oct 10, 2018

Improve support for aspects (feature that is allowed to exist in ActivityPub, and is implemented in Hubzilla and possibly other implementations). Allowed recipients are explicitly listed in the audience fields.

Before: A message with explicitly listed recipients would be delivered as a DM notification to all those recipients, but only as long as those were also tagged.

After: Save silent mentions, i.e. those that appear in to/cc but not in tag for access control, do not create a notification for silent mentions, push those into home feeds instead (only if the author is followed by the recipients), and ideally mark the post as "private" instead of "direct" in the REST API.

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2018

Having it show as private might lead the receiving person to think every one of the author's followers are allowed to see that information.

Also, what about mentioned accounts that are not part of the audience?

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

  • The look of the UI does not affect internal behaviour
  • These messages are closer to private than DM in terms of UX (i.e. less personal, not to be highlighted)
  • Adding a new visibility level would be a big API change in regards to apps

Whatever happens internally, it is easier to mask as "private" in the API to get the desired UX.

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

Also, what about mentioned accounts that are not part of the audience?

https://github.com/tootsuite/mastodon/blob/master/app/lib/activitypub/activity.rb#L100

@trwnh

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

push those into home feeds instead (only if the author is followed by the recipients)

I think I understand the reasoning for this, but there is one concern: if a message is shared without a mention, as is possible in other systems, then how does a Mastodon user know to follow the sender in order to receive the message?

Typically in those other social models, a notification is generated for "x has started sharing with you". This lets you know that someone has made you part of an audience, and you can then choose to receive those posts. But because Mastodon uses a follow-first model instead of a share-first model, only follower notifications are currently generated.

My interpretation would be that there should be an "incoming share requests" section, similar to "follower requests". This "incoming" section would show either the messages themselves directly as a column, or it would show just the profiles listed with a follow button.

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

Typically in those other social models, a notification is generated for "x has started sharing with you". This lets you know that someone has made you part of an audience, and you can then choose to receive those posts. But because Mastodon uses a follow-first model instead of a share-first model, only follower notifications are currently generated.

My interpretation would be that there should be an "incoming share requests" section, similar to "follower requests". This "incoming" section would show either the messages themselves directly as a column, or it would show just the profiles listed with a follow button.

Not interested in that at all. It's just a venue for spam. Marketing account sends share requests to every single person in the fediverse etc etc. The idea behind aspects/circles is that you choose which of your followers you share something with, not that you just arbitrarily pick random people to share with. Non-followers do not need to be supported in Mastodon.

@Gargron Gargron force-pushed the feature-support-aspects branch from e913bef to e74bda9 Oct 10, 2018
@trwnh

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

Non-followers do not need to be supported in Mastodon.

OK, this definitely matches up with my expectation. But might there be a difference between to and cc interpretations? If to is direct targeting and cc is visibility targeting, then would that logic apply only to rejecting messages from non-followers with cc: you? Or would messages to: you missing a mention still show up (in your notifications and/or your direct messages)? Related: #7394, #8067

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

I do not think that there is any meaningful difference between to and cc when we're not talking about a magic value like the public collection, that's why process_audience clumps them together. The deciding factor is the presence of a tag. I believe that this will work alright.

Gargron added 5 commits Oct 10, 2018
Move networking calls out of the database transaction
Unlike DMs, limited statuses are pushed into home feeds. The access
control rules between direct and limited statuses is almost the same,
except for counter and conversation logic
As those are "this person is also allowed to see" rather than "this
person is involved", therefore does not warrant filtering
@Gargron Gargron force-pushed the feature-support-aspects branch from 769e7ba to d787fd8 Oct 10, 2018
app/lib/activitypub/activity/create.rb Outdated Show resolved Hide resolved
app/lib/activitypub/activity.rb Outdated Show resolved Hide resolved
app/lib/feed_manager.rb Outdated Show resolved Hide resolved
app/lib/feed_manager.rb Outdated Show resolved Hide resolved
app/models/mention.rb Outdated Show resolved Hide resolved
app/serializers/activitypub/note_serializer.rb Outdated Show resolved Hide resolved
app/serializers/rest/status_serializer.rb Outdated Show resolved Hide resolved
app/workers/activitypub/reply_distribution_worker.rb Outdated Show resolved Hide resolved
@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2018

Also, what about mentioned accounts that are not part of the audience?

https://github.com/tootsuite/mastodon/blob/master/app/lib/activitypub/activity.rb#L100

So this doesn't push the message to those users' feeds, but they are still able to see them (by viewing the thread, the poster's toots, etc.)?

Gargron added 2 commits Oct 11, 2018
@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2018

@ThibG Yes, I think so.

@ykzts
ykzts approved these changes Oct 16, 2018
And remove stream_entry eager-loading from Notification
@Gargron Gargron force-pushed the feature-support-aspects branch from 5fdb939 to 398c4be Oct 17, 2018
@Gargron Gargron dismissed nightpool’s stale review Oct 17, 2018

Addressed

@Gargron Gargron merged commit ddd30f3 into master Oct 17, 2018
11 checks passed
11 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
@Gargron Gargron deleted the feature-support-aspects branch Oct 17, 2018
Gargron added a commit that referenced this pull request Oct 17, 2018
Fix regression from #8950
Gargron added a commit that referenced this pull request Oct 17, 2018
)

* Do not show "limited" visibility in default visibility preference

Fix regression from #8950

* Fix code style issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.