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

Labels: Added complex filtering logic #4449

Merged
merged 13 commits into from
Feb 6, 2024
Merged

Conversation

ebeers-png
Copy link
Collaborator

What's this PR do?

Adds more complex label filtering logic to the inventory list, by allowing users to filter the inventory by label on AND, OR, and EXCLUDE all at once.

The complex label logic can be saved to filter groups, and migrations will update pre-existing filter groups to the new setup.

Remaining work

Before merging this into develop, the inventory list menu should be re-organized to make room for the two new rows, which take up a lot of screen space.

The inventory map page, which also uses label filtering, was left unchanged, since it doesn't (yet) use Filter Groups and will work fine without the complex label logic. But it can be easily updated if needed, since its own code is already based on the inventory list's.

What are the relevant tickets?

#3371

Screenshots (if appropriate)

(Ignore the missing icons)

image

@ebeers-png ebeers-png added Filtering Inventory List Enhancement Add this label if functionality was generally improved but not a full feature or maintentance. Clearly Energy BEAM Implemented labels Dec 19, 2023
Copy link
Contributor

@haneslinger haneslinger left a comment

Choose a reason for hiding this comment

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

This is great! thank you! I have only a few notes, and a tiny commit that adds a test I would like to see passing. I'm happy to address them, if you like.

@kflemin, how to you want to handle that white space in the ui @ebeers-png mentioned above?

@ebeers-png, Have you noticed any different in query speed?

Comment on lines 260 to 261
or_views = set(cycle.propertyview_set.filter(labels__in=or_labels))
views = set.intersection(views or or_views, or_views)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I'm missing something, but wouldn't this suffice?

Suggested change
or_views = set(cycle.propertyview_set.filter(labels__in=or_labels))
views = set.intersection(views or or_views, or_views)
views = views.filter(labels__in=or_labels)

Comment on lines 263 to 264
exclude_views = set(cycle.propertyview_set.exclude(labels__in=exclude_labels))
views = set.intersection(views or exclude_views, exclude_views)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Suggested change
exclude_views = set(cycle.propertyview_set.exclude(labels__in=exclude_labels))
views = set.intersection(views or exclude_views, exclude_views)
views = views.exclude(labels__in=exclude_labels)

Comment on lines 254 to 258
views_all = []
for label in labels:
for label in and_labels:
views = cycle.propertyview_set.filter(labels__in=[label])
views_all.append(views)
return list(set.intersection(*map(set, views_all)))
elif logic == 1: # or
return list(cycle.propertyview_set.filter(labels__in=labels))
elif logic == 2: # exclude
return list(cycle.propertyview_set.exclude(labels__in=labels))
views = set.intersection(*map(set, views_all))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this suffice?

views = views.filter(**{"labels__in": l for l in and_label})

Comment on lines 141 to 151
const and_label_ids = [];
const or_label_ids = [];
const exclude_label_ids = [];
for (const label of $scope.selected_and_labels) {
and_label_ids.push(label.id);
}
for (const label of $scope.selected_or_labels) {
or_label_ids.push(label.id);
}
for (const label of $scope.selected_exclude_labels) {
exclude_label_ids.push(label.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const and_label_ids = [];
const or_label_ids = [];
const exclude_label_ids = [];
for (const label of $scope.selected_and_labels) {
and_label_ids.push(label.id);
}
for (const label of $scope.selected_or_labels) {
or_label_ids.push(label.id);
}
for (const label of $scope.selected_exclude_labels) {
exclude_label_ids.push(label.id);
const and_label_ids = scope.selected_and_labels.map(l => l.id);
const or_label_ids = scope.selected_or_labels.map(l => l.id);
const exclude_label_ids = scope.selected_exclude_labels.map(l => l.id);

Comment on lines 265 to 287
const current_and_labels = [];
const current_or_labels = [];
const current_exclude_labels = [];
for (const label of $scope.selected_and_labels) {
current_and_labels.push(label.id);
}
const saved_labels = $scope.currentFilterGroup.labels;
const current_label_logic = $scope.labelLogic;
const saved_label_logic = $scope.currentFilterGroup.label_logic;
for (const label of $scope.selected_or_labels) {
current_or_labels.push(label.id);
}
for (const label of $scope.selected_exclude_labels) {
current_exclude_labels.push(label.id);
}
saved_and_labels = $scope.currentFilterGroup.and_labels;
saved_or_labels = $scope.currentFilterGroup.or_labels;
saved_exclude_labels = $scope.currentFilterGroup.exclude_labels;
if (!_.isEqual(current_filters, saved_filters)) {
$scope.Modified = true;
} else if (!_.isEqual(current_labels.sort(), saved_labels.sort())) {
} else if (!_.isEqual(current_and_labels.sort(), saved_and_labels.sort())
|| !_.isEqual(current_or_labels.sort(), saved_or_labels.sort())
|| !_.isEqual(current_exclude_labels.sort(), saved_exclude_labels.sort())) {
$scope.Modified = true;
} else {
$scope.Modified = current_label_logic !== saved_label_logic;
$scope.Modified = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const current_and_labels = [];
const current_or_labels = [];
const current_exclude_labels = [];
for (const label of $scope.selected_and_labels) {
current_and_labels.push(label.id);
}
const saved_labels = $scope.currentFilterGroup.labels;
const current_label_logic = $scope.labelLogic;
const saved_label_logic = $scope.currentFilterGroup.label_logic;
for (const label of $scope.selected_or_labels) {
current_or_labels.push(label.id);
}
for (const label of $scope.selected_exclude_labels) {
current_exclude_labels.push(label.id);
}
saved_and_labels = $scope.currentFilterGroup.and_labels;
saved_or_labels = $scope.currentFilterGroup.or_labels;
saved_exclude_labels = $scope.currentFilterGroup.exclude_labels;
if (!_.isEqual(current_filters, saved_filters)) {
$scope.Modified = true;
} else if (!_.isEqual(current_labels.sort(), saved_labels.sort())) {
} else if (!_.isEqual(current_and_labels.sort(), saved_and_labels.sort())
|| !_.isEqual(current_or_labels.sort(), saved_or_labels.sort())
|| !_.isEqual(current_exclude_labels.sort(), saved_exclude_labels.sort())) {
$scope.Modified = true;
} else {
$scope.Modified = current_label_logic !== saved_label_logic;
$scope.Modified = false;
current_and_labels = new Set($scope.selected_and_labels.map(l => l.id));
current_or_labels = new Set($scope.selected_and_labels.map(l => l.id));
current_exclude_labels = new Set($scope.selected_and_labels.map(l => l.id));
saved_and_labels = new Set($scope.currentFilterGroup.and_labels);
saved_or_labels = new Set($scope.currentFilterGroup.or_labels);
saved_exclude_labels = new Set($scope.currentFilterGroup.exclude_labels);
$scope.Modified = !(
_.isEqual(current_and_labels, saved_and_labels) &&
_.isEqual(current_or_labels, saved_or_labels) &&
_.isEqual(current_exclude_labels, saved_exclude_labels)
)

Comment on lines 124 to 129
if "and_labels" in request.data:
filter_group.and_labels.set(request.data["and_labels"])
if "or_labels" in request.data:
filter_group.or_labels.set(request.data["or_labels"])
if "exclude_labels" in request.data:
filter_group.exclude_labels.set(request.data["exclude_labels"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this catch erroneous data?

@@ -0,0 +1,40 @@
# Generated by Django 3.2.18 on 2023-12-19 16:15
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@ebeers-png
Copy link
Collaborator Author

@haneslinger Thank you for the review and suggestions! Added the catch for erroneous data to the update method, and I was able to simplify _get_label_views() a little more as well.

Re: query speed, I haven't really noticed a difference in inventory load times. But that's mainly because SEED calculates included/excluded building ids on the frontend, in inventory_list_controller (lines 1110-1128). That's not the case for the custom report insights though, which uses _get_label_views() in models/data_views to get the right buildings. So I'd have to check whether the old code (with set.intersection(...) everywhere) is faster than what you suggested (though I doubt it).

@haneslinger
Copy link
Contributor

The failing test is not this PRs fault. I'm addressing it now

@haneslinger haneslinger merged commit d5d60ea into develop Feb 6, 2024
8 checks passed
@haneslinger haneslinger deleted the complex_filtering branch February 6, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BEAM Implemented Clearly Energy Enhancement Add this label if functionality was generally improved but not a full feature or maintentance. Filtering Inventory List
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants