Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

Support for 'any' vs 'all' as well as 'not' filtering for tags #399

Closed
wants to merge 31 commits into from
Closed

Support for 'any' vs 'all' as well as 'not' filtering for tags #399

wants to merge 31 commits into from

Conversation

shamoon
Copy link
Contributor

@shamoon shamoon commented Jan 21, 2021

This PR addresses part of #356 as well as closes #401 by adding support for changing tag filtering from its current "All" to allow "Any" as well as adding support for NOT (option-click).

any_all_not.mov

@jonaswinkler I found tags__id__in in the API so thought it wouldn't be such a big deal but one thing is that the API is returning duplicate documents when they have multiple tags (e.g. document has tag 1 & tag 2, search for tag 1 or tag 2 it returns that doc twice). Im no Django expert, thought I could resolve with something like:
Thoughts on that issue (or other feedback of course)?

Edit: think that issue is sorted, welcome your feedback in general

@shamoon
Copy link
Contributor Author

shamoon commented Jan 21, 2021

Oh, duh, distinct()!

Ok, let me know what you think when you have a moment.

@shamoon shamoon marked this pull request as ready for review January 21, 2021 02:35
@@ -199,15 +204,15 @@ export class FilterEditorComponent implements OnInit, OnDestroy {
}

toggleTag(tagId: number) {
this.tagSelectionModel.toggle(tagId)
this.tagSelectionModel.set(tagId, ToggleableItemState.Selected)
Copy link
Contributor Author

@shamoon shamoon Jan 21, 2021

Choose a reason for hiding this comment

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

These are a fix that I just included, clicking a tag/dt/corresp should always enable that item, not remove it (because if that tag was already selected the document you just clicked disappears which doesnt make sense IMHO)

@jonaswinkler
Copy link
Owner

See aeb10d3

@jonaswinkler
Copy link
Owner

I'd also like to consider this: #401

@shamoon
Copy link
Contributor Author

shamoon commented Jan 21, 2021

Hmm, increasing complexity for our humble dropdowns. So I guess for "not" we could implement something similar with the ⌥ modifier key (though as in the GH UI you have to tell the user somehow since its totally not obvious). But this feature could also just remain as-is. So you can switch from "label 1 AND not label2" to "label 1 OR not label2". Right?

Would you prefer adding that into this PR or merging this one and I can work on "not" separately? (version control wise I think smaller PRs are better but thats just my opinion, happy either way).

@shamoon
Copy link
Contributor Author

shamoon commented Jan 21, 2021

TBH I'm not sure exactly how to tackle "not", I have to think about it some. You'd need something like a filter rule for each selected tag +/- that are then joined by AND vs OR but then it'd be like "(tag
1 OR not tag2) AND correspondent1" etc right? But currently OR for tags is 1 rule...

@jonaswinkler
Copy link
Owner

jonaswinkler commented Jan 21, 2021

Hmm, increasing complexity for our humble dropdowns

I don't like it either, will have to think about that.

So you can switch from "label 1 AND not label2" to "label 1 OR not label2". Right?

I'm not sure yet how compatible these two features are. Will have to think about that. Querying for "label1 OR not label2" is most certainly not possible. will require new filtering mechanisms, and the logic for that isn't very straight forward. I'm concerned that the logic of the drop downs won't be very intuitive with both these feature.

@shamoon
Copy link
Contributor Author

shamoon commented Jan 21, 2021

Yeah at some point we have to ask ourselves if cramming everything into this one place is the best way to do it. But I also don't love the idea of having to support an entirely different place for managing "advanced" filters or something like that.

Without real 'evidence' my suspicion is that "any/all" would get more use than "not" as its been requested more than once and I have on occasion found myself looking for that in my own usage.

But yea, TBD...

@shamoon
Copy link
Contributor Author

shamoon commented Jan 21, 2021

Sorry for all the comments. One other thought would be to just disable the "any/all" toggle if any tags are set to "not". Obviously still the challenge of implementing "not" in general, but this would reduce the complexity overall

@shamoon
Copy link
Contributor Author

shamoon commented Jan 21, 2021

Hey FYI I have a branch that has "not" working pretty well (needs a little tweaking): https://github.com/shamoon/paperless-ng/tree/fix/issue-401

Let me know thoughts about these two features and we can figure out from there.

@shamoon
Copy link
Contributor Author

shamoon commented Jan 21, 2021

I saw you pushed 1.0, nice!

One last update: I merged my branch for "not" into this one (I haven't committed this cause will wait on your thoughts) and honestly I think it works well. I have the any / all toggle disable if items are excluded. It seems like a pretty specific subset of use cases where someone wants the ability to "tag1 OR not tag2" and I think its better to ship these two features than spend time on all that for that small gain.

Let me know, I can commit these changes so you can test everything together or whatever you think.

@shamoon
Copy link
Contributor Author

shamoon commented Jan 23, 2021

Went ahead and pushed commits described above. Let me know if any issues with this approach or if youre just too dang busy I get it!

@shamoon shamoon changed the title Support for 'any' vs 'all' filtering for tags Support for 'any' vs 'all' as well as 'not' filtering for tags Jan 23, 2021
@jonaswinkler
Copy link
Owner

Thanks for all the effort! I still need to look into this.

Without real 'evidence' my suspicion is that "any/all" would get more use than "not" as its been requested more than once and I have on occasion found myself looking for that in my own usage

Not sure. I've seen both mentioned one or two times, which is not alot.

Also: Not sure how this will play out with nested tags. I'd say it's wise to postpone this PR until that's figured out.

@shamoon
Copy link
Contributor Author

shamoon commented Jan 23, 2021

Also: Not sure how this will play out with nested tags. I'd say it's wise to postpone this PR until that's figured out.

Obviously your call, you da boss. I didn't realize nested tags was this close but I guess we're at 1.0 so =)
In my very rough conception of nested tags I imagined they'd be essentially a parent is like an "alias" to children, so searching for parent id also just includes its children (at the API level). In that case having any / all and NOT still wouldn't really be a problem i.e. you can search for parent but exclude one of its children, no? UI-wise there are definitely going to be some challenges / decisions to make. Anyway, no reason to get into the weeds here.

As I mentioned in other issues, I enjoy helping out so let me know where I might be useful with nested tags or whatever else.

@@ -15,6 +15,8 @@ export class FilterableDropdownSelectionModel {
changed = new Subject<FilterableDropdownSelectionModel>()

multiple = false
private _logicalOperator = 'and'
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Strings must use doublequote.

Suggested change
private _logicalOperator = 'and'
private _logicalOperator = "and"

@@ -43,6 +45,10 @@ export class FilterableDropdownSelectionModel {
return this.items.filter(i => this.temporarySelectionStates.get(i.id) == ToggleableItemState.Selected)
}

getExcludedItems() {
return this.items.filter(i => this.temporarySelectionStates.get(i.id) == ToggleableItemState.Excluded)
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected parentheses around arrow function argument.

Suggested change
return this.items.filter(i => this.temporarySelectionStates.get(i.id) == ToggleableItemState.Excluded)
return this.items.filter((i) => this.temporarySelectionStates.get(i.id) == ToggleableItemState.Excluded)

tribut and others added 8 commits August 26, 2021 18:40
When set, the user is redirected to this URL after a logout. Especially
useful in conjunction with PAPERLESS_ENABLE_HTTP_REMOTE_USER and SSO.
This fixes the ansible role for installing paperless-ng where the config
item PAPERLESS_TIKA_GOTENBERG_ENDPOINT derived from the wrong ansible
variable "paperlessng_tika_endpoint".  This patch corrects that to
"paperlessng_tika_gotenberg_endpoint".
Clarified exactly what to change to modify the default webserver port (issue #1273)
Updated docker instructions re webserver port
…ndpoint

Fix PAPERLESS_TIKA_GOTENBERG_ENDPOINT in a/t/main.yml
Add PAPERLESS_LOGOUT_REDIRECT_URL
Incorrect path to docker-compose files
@martin-v
Copy link

Thanks for your work! @shamoon @jonaswinkler but is there any change to get this merged? It would be still an important feature.

@shamoon shamoon closed this Jun 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants