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

Allow joining several hashtags in a single column #8904

Merged
merged 35 commits into from Nov 5, 2018

Conversation

Projects
None yet
6 participants
@gdpelican
Copy link
Collaborator

gdpelican commented Oct 7, 2018

Ref #6359

Watch this video for more details on intended functionality :)

@gdpelican gdpelican force-pushed the gdpelican:multi-tag-columns branch from c8d5ace to 8b2bdcf Oct 7, 2018

@ashleyhull-versent

This comment has been minimized.

Copy link
Contributor

ashleyhull-versent commented Oct 7, 2018

Excellent video explainer... does this limit to hashtags filtered from the home feed and/or local feed and/or federated feed?

this feature is worth it on it's own, but I'd like to see hashtags being pulled from the fediverse to get access to toots in and out of my social circle. if possible, I'd be nice to be able to filter in or out either locally or from the firehose.

@gdpelican

This comment has been minimized.

Copy link
Collaborator Author

gdpelican commented Oct 7, 2018

I have to admit to still be wrapping my head around the federation features of Mastodon, but this PR always respects 'local' variable that's passed through on the backend.

So I believe the answer is 'If your normal hashtag column displays remote tweets, then the modified AND/OR/NOT column will as well', but there's nothing in here at the moment around going and fetching statuses from other instances, which would likely be out of scope for this particular PR.

@ashleyhull-versent

This comment has been minimized.

Copy link
Contributor

ashleyhull-versent commented Oct 7, 2018

Totally understandable... thanks :)

@gdpelican gdpelican referenced this pull request Oct 9, 2018

Closed

Join several hashtags in a search/column #6359

2 of 2 tasks complete
@gdpelican

This comment has been minimized.

Copy link
Collaborator Author

gdpelican commented Oct 20, 2018

Alright, thanks for the feedback guys.

I've had a swing at styling the selects, and done a refactor so that the tags can be applied in all 'modes' at once (full power!)

image

image

I've also put those SQL injection warnings into the brakeman file, and changed the code up a little bit so it's a bit more obvious there's no injection to be had there (we now call to_i on the input before it goes in)

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Oct 20, 2018

Wait, how does the streaming part work?

@Gargron
Copy link
Member

Gargron left a comment

Sorry, reviewed it again and found some issues. Most importantly, the streaming API is untouched, so even with all the column params, new statuses will only appear from the main hashtag.

Changing the streaming API to support such filtering is not straightforward and I don't have advice ready. Each streaming API consumer subscribes to one redis pubsub channel (well, the channels are multiplexed actually). But the the thing is, you can't really subscribe to a union of two channels. They aren't published in the same server tick either, there might be any delay.

Perhaps your only option is to subscribe to all individual streaming API channels in the web UI, and then filter on ingress based on the tags included in the JSON.

@@ -86,6 +87,10 @@ const deleteStatus = (state, id, accountId, references) => {
return state;
};

const clearTimeline = (state, timeline) => {
return state.updateIn([timeline, 'items'], list => list.filter(() => false));

This comment has been minimized.

@Gargron

Gargron Oct 20, 2018

Member

list => list.clear()

@@ -79,6 +79,17 @@ class Status < ApplicationRecord
scope :including_silenced_accounts, -> { left_outer_joins(:account).where(accounts: { silenced: true }) }
scope :not_excluded_by_account, ->(account) { where.not(account_id: account.excluded_from_timeline_account_ids) }
scope :not_domain_blocked_by_account, ->(account) { account.excluded_from_timeline_domains.blank? ? left_outer_joins(:account) : left_outer_joins(:account).where('accounts.domain IS NULL OR accounts.domain NOT IN (?)', account.excluded_from_timeline_domains) }
scope :tagged_with_all, ->(tags) {

This comment has been minimized.

@Gargron

Gargron Oct 20, 2018

Member

I guess stylistically if you need a multiline lambda for a scope, the scope is better off being a class method

This comment has been minimized.

@gdpelican

gdpelican Oct 20, 2018

Author Collaborator

There's a subtle difference here in that in a class method, self refers to the class itself (ie, Status, which would map to Status.all), whereas within a scope self refers to the existing relation that the scope's being called on.

So a straight port to a class method doesn't quite work because instead of reducing the existing relation, we start reducing from all statuses

@@ -16,14 +16,16 @@ def show
end

format.rss do
@statuses = Status.as_tag_timeline(@tag).limit(PAGE_SIZE)
@statuses = HashtagQueryService.new.call(@tag, params.slice(:any, :all, :none)).distinct.limit(PAGE_SIZE)

This comment has been minimized.

@Gargron

Gargron Oct 20, 2018

Member

If distinct is called on all uses of the service, why not put it inside the service?

This comment has been minimized.

@gdpelican

gdpelican Oct 20, 2018

Author Collaborator

Ah, it was because rails 5's or method is verrrry picky about what kinds of clauses you can join via or (and distinct was blowing it up), but I believe we can pull it into the service by calling distinct on both the original query and the 'or' one.

@gdpelican

This comment has been minimized.

Copy link
Collaborator Author

gdpelican commented Oct 20, 2018

Okay great, I've patched up a couple little things you mentioned and will have a poke into the streaming stuff over the next while.

An initial glance makes me think there are two possible solutions:

  • Define a specific stream which includes the additional arguments... right now it's hashtag?tag=cats and maybe it could be hashtag?tag=cats&any[]=dogs. The streamFrom method on the streaming server is still a bit opaque to me, but I'd think this approach would be better if we can get it. EDIT: Oh right redis streams, heh. Hmmm...

  • Have the column also subscribe to everything in the 'any' field, and then discard clientside anything that comes through which violates the 'and' or 'not' fields

@trwnh

This comment has been minimized.

Copy link
Contributor

trwnh commented Oct 20, 2018

The screenshot you posted looks better, but I'm still wondering what would happen if you wanted to remove the "cats" hashtag while keeping all the other tags. Would you have to make an entirely new column? This would be inconvenient if you had many "additional" tags but only wanted to change the "primary".

@gdpelican

This comment has been minimized.

Copy link
Collaborator Author

gdpelican commented Oct 20, 2018

@trwnh Hm, I'm not really sure I'm following you here; there's not currently a function that I'm aware of to update the hashtag of a column (I suppose the workflow is 'make a new column, pin it, and unpin the old one'?), and I'm not sure it's within the scope of this PR to add it, since this is simply trying to add options to an existing column.

You could actually do some odd stuff with these settings like 'cats OR dogs WITHOUT cats', which would end up being a slightly-less-performant 'dogs' column, but I wouldn't really want to encourage that behaviour.

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Oct 20, 2018

I mean, tagId is just a prop passed from column settings in the settings reducer, so it could be changed on the fly, I think

gdpelican added some commits Oct 21, 2018

@gdpelican

This comment has been minimized.

Copy link
Collaborator Author

gdpelican commented Oct 21, 2018

Alright, I've updated this to do the second option here, which is

  • When subscribing a hashtag column, subscribe to the 'main' tag, as well as all of the tags in the 'any' field
  • When a status comes through one of those subscriptions, run it through an 'accept' function which will ignore it if it doesn't meet the criterion of either the 'all' or 'none' fields.

@Gargron Gargron added the ui label Oct 23, 2018

Addresed

@justinmayer

This comment has been minimized.

Copy link

justinmayer commented Oct 23, 2018

Great work on this feature, James. In addition, I like the inclusion of the video to help introduce the use case and initial implementation. Bravo! 🚀

@gdpelican

This comment has been minimized.

Copy link
Collaborator Author

gdpelican commented Nov 2, 2018

Hey @Gargron , is there anything else I can do here to shore this one up?

@Gargron

Gargron approved these changes Nov 2, 2018

@Gargron Gargron merged commit 4c03e05 into tootsuite:master Nov 5, 2018

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
@gdpelican

This comment has been minimized.

Copy link
Collaborator Author

gdpelican commented Nov 6, 2018

Thanks for the support on this @Gargron; I had a great first PR experience with Mastodon. Feel free to ping me if there are any issues or enhancements that come up around this and I'll be happy to take a look.

@mayaeh mayaeh referenced this pull request Nov 11, 2018

Merged

i18n: Add ja translations #9261

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 13, 2018

It takes more than 4 seconds to load "fediplay OR np" so something about these queries isn't quite right.

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 13, 2018

Oh and it seems to reload the column every time you navigate anywhere. I should open an issue for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment