I started looking into implementing this feature and found some oddities related to how the combination of Slack subscriptions for categories and tags work. I submitted a PR for one issue, but can see some additional ones that we may need to tackle.
Currently, a Slack plugin subscription in the UI makes it look like you can choose a category and a set of tags for a single filter. But in my tests, it looks like any time you add a tag, it sets the category to “all” when you save the filter.
I think this is OK, since nowhere else in the application do we allow you to combine a category and set of tags in a single filter (eg, user notification preferences).
Can I proceed with the assumption that we don’t actually want to allow a user to choose a category other than “all” in a filter that includes tags?
As you mentioned we already allowing users to choose both category and tags combination in admin UI. It means we are allowing users to choose the below combinations
Notification will be sent on all new topic creations when
All Categories + No Tags => User can see the topic
All Categories + TAG1 => Topic with tag TAG1 + User can see
CATEGORY1 + No tags => Topic in category CATEGORY1 + User can see
CATEGORY1 + TAG1 => Topic in category CATEGORY1 with tag TAG1 + User can see
Ok, I will make sure to preserve that behavior in this case.
But still wondering if this adds enough value to maintain in the long run:
I can provide some examples where is it further complicates the UX.
For instance, let’s say I have a filter that is: (cat1, [tag1], following). Then I had a filter that is (cat1, [tag2], watching). Are these one filter or two at this point? Let’s assume 2.
Then I change the 2nd filter from watching to following. Should it then be combined and leave me with [cat1, [tag1, tag2], following)?
These problems are solvable, but I’m just not sure it’s worth it in the long run.
It is @tgxworld’s recommended way and I like it. Because it gives all the possibilities to the filter rules.
Yes it is little bit complicates. While testing your example I found a bug again now. But I hope it is worth it and useful to other plugin users @codinghorror@tgxworld?
More concretely, I think it may make sense to work towards a design where a subscription is analogous to a user’s notification preferences. But instead of the user_id being the key, the Slack channel would be the key.
Each Slack channel would have at most 1 subscription.
Updating a channel’s subscription would just mutate one record, and the code would enforce that a given category or tag can only belong to one of the respective lists. Notifications would match topics to a subscription filter based on whether the category OR any tags match, and continue to use the existing logic for precedence of muted over watching and following.
Hi @mcwumbly. I’m working on the new discourse-chat-integration plugin, so have spent quite a lot of time over the last couple of weeks re-implementing the filtering system from the slack plugin.
In doing so, I’ve ironed out (hopefully) all of the “oddities” you described at the start of this topic. I only just found this topic, hence why I didn’t post before starting work!
While I like the idea of 1 record for each channel, I can’t see how it work work for the case:
If we are keeping the feature to combine categories and tags in a single filler, then doing something closer to what you’ve described makes sense. I think this may introduce more complexities in the UX, particularly when it comes to updating them via Slack commands, though, and I personally question whether it’s worth it.
If we are keeping the ability to have categories & tags in one rule, then this is the UX I’ve been working on within slack. It’s based on the existing slack plugin, with some adaptations which I hope make it better
/discourse watch some-category
would just add the rule to the list, sorted in order of precedence (topmost matching rule wins).
You can also have multiple tags in the same command /discourse watch some-category tag:food tag:new
And completely miss out the category if you like (‘all categories’ is assumed) /discourse watch tag:test
Then deleting the second rule in the list is as simple as /discourse remove 2
I think having simple add/remove rules options is a simpler UX than the existing interface (which tries to work out how to augment the existing rules in order to match your command). It also matches the admin UI exactly which should help. There would still be some simple logic to avoid duplicates.
What do you think? Does that help with the UX complexities you describe?
If you see value in supporting the ability to create a filter with multiple tags in one command, I might suggest this syntax instead though, which more closely matches how things are displayed:
/discourse watch some-category tags:foo,bar,baz
One way to think about the current implementation is that a given tuple (category, tag) can only have one status associated with it from the following set: (watching, following, muted, nil). And a user can only modify a single tuple at a time. (This is where the “it tries to work out how to augment the existing rules” comes in. But I don’t think it’s actually too difficult to reason about from the user’s perspective).
To make things simpler, the Slack command UX currently only allows you to create or update filters where category or tag is nil. But thinking about it more now, I don’t think it would be too difficult or add too much additional complexity to support the combination of category/tag filters in the current design.
That said, I’m not attached to it if you find that the design you’re currently exploring is the way things should go.
I would recommend exploring this a bit more since I agree that seems like the biggest change in your new design. See if you can think of likely ways that people may end up with filters that have overlapping matches and what, if anything, the plugin should do to try to help there.
You might also want to take a peek at the code for the plugin as it exists in my current fork as I think the implementation for the rule augmentation has been simplified quite a bit, but this pull request is still pending.
Will have a look at this over the next week.