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.

Sign up for GitHub

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

Jump to bottom

Add featured hashtags as an ActivityPub collection #11595

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Add featured hashtags as an ActivityPub collection #11595

merged 1 commit into from
Sep 2, 2020

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Aug 18, 2019 •
edited
Loading

Resolve #10478

New collection featuredTags containing Hashtag objects.

@Gargron Gargron added the activitypub Protocol-related changes, federation label Aug 18, 2019
@Gargron Gargron force-pushed the feature-featured-tags-activitypub branch from d26e2d6 to 8d91484 Compare August 18, 2019 01:49
@ClearlyClaire
Copy link
Contributor

While from a protocol standpoint this sounds somewhat fine to me, I'm pretty sure this will break any current Mastodon instance.

Indeed, Mastodon currently assume every item is either a Note's URI, or an object with a Note's URI:

https://github.com/tootsuite/mastodon/blob/4e921832272425352d28cad550bfc4dffd6d0e78/app/services/activitypub/fetch_featured_collection_service.rb#L25-L30

It may be best to have different collections for this

@Gargron
Copy link
Member Author

Gargron commented Aug 18, 2019

@ThibG Since the Mention and Hashtag objects do not have an id property and are not strings, the initial map will return nil for those items. Then the nil will be passed to ActivityPub::FetchRemoteStatusService, which should, in turn, return nil, which would finally be removed by compact. Isn't that fine? It means older versions won't break?

@Gargron
Copy link
Member Author

Gargron commented Aug 18, 2019

Correction: Passing nil into ActivityPub::FetchRemoteStatusService throws an ArgumentError from the Request class. Very unfortunate. It would have worked if not for that one bit. :(

@noellabo noellabo mentioned this pull request Sep 13, 2019
Merged
@stale
Copy link

stale bot commented Oct 26, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/wontfix This will not be worked on label Oct 26, 2019
@stale stale bot closed this Nov 2, 2019
@trwnh trwnh mentioned this pull request Jan 6, 2020
Closed
@ykzts ykzts deleted the feature-featured-tags-activitypub branch January 25, 2020 04:44
@Gargron Gargron restored the feature-featured-tags-activitypub branch August 31, 2020 21:36
@Gargron Gargron reopened this Aug 31, 2020
@stale stale bot removed the status/wontfix This will not be worked on label Aug 31, 2020
@Gargron Gargron force-pushed the feature-featured-tags-activitypub branch 2 times, most recently from 0cb5935 to 785d022 Compare August 31, 2020 22:00
@Gargron Gargron changed the title Add featured hashtags and endorsed accounts to ActivityPub featured collection Add featured hashtags as an ActivityPub collection Aug 31, 2020
@Gargron Gargron force-pushed the feature-featured-tags-activitypub branch from 785d022 to e2c8569 Compare August 31, 2020 22:01
@Gargron
Copy link
Member Author

Gargron commented Aug 31, 2020

I've decided to rework this to use a separate collection after all. I do not like making up another ActivityPub vocabulary term but maybe it's for the best 🤷‍♂️

ClearlyClaire
ClearlyClaire approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made a few inline comments, but none of them prevent this PR from being merged.

app/controllers/activitypub/collections_controller.rb
@@ -12,25 +12,17 @@ class ActivityPub::CollectionsController < ActivityPub::BaseController

def show
expires_in 3.minutes, public: public_fetch_mode?
render_with_cache json: collection_presenter, content_type: 'application/activity+json', serializer: ActivityPub::CollectionSerializer, adapter: ActivityPub::Adapter, skip_activities: true
render_with_cache json: collection_presenter, content_type: 'application/activity+json', serializer: ActivityPub::CollectionSerializer, adapter: ActivityPub::Adapter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip_activities doesn't do anything anymore? when have we removed it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't even find where it was used... It doesn't seem to appear anywhere else in the code.

app/controllers/activitypub/collections_controller.rb
end
@items = for_signed_account { cache_collection(@account.pinned_statuses, Status) }
when 'tags'
@items = for_signed_account { @account.featured_tags }
when 'devices'
@items = @account.devices
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this case could use for_signed_account too although it's not really a big deal either.

@Gargron Gargron merged commit 1734036 into master Sep 2, 2020
@Gargron Gargron deleted the feature-featured-tags-activitypub branch September 2, 2020 00:12
thenameisnigel-old pushed a commit to ChatterlyOSE/Chatterly that referenced this pull request Sep 6, 2020
@Gargron @thenameisnigel-old
Add featured hashtags as an ActivityPub collection (mastodon#11595)
b9d5d34
thenameisnigel-old pushed a commit to ChatterlyOSE/Chatterly that referenced this pull request Sep 7, 2020
@Gargron @thenameisnigel-old
Add featured hashtags as an ActivityPub collection (mastodon#11595)
97a7adb
@noellabo noellabo mentioned this pull request Oct 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClearlyClaire ClearlyClaire ClearlyClaire approved these changes

@ykzts ykzts ykzts approved these changes

Assignees
No one assigned
Labels
activitypub Protocol-related changes, federation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gargron @ClearlyClaire @ykzts