Fix includes all query returning duplicates
This commit is contained in:
+28
-6
@@ -395,13 +395,26 @@ class PurchaseFilter(OperatorFilter):
|
|||||||
join and would require a single link row to be both games. Instead
|
join and would require a single link row to be both games. Instead
|
||||||
chain a filter per game so each gets its own join, then match by
|
chain a filter per game so each gets its own join, then match by
|
||||||
``pk``. ``INCLUDES_ONLY`` additionally excludes purchases that have
|
``pk``. ``INCLUDES_ONLY`` additionally excludes purchases that have
|
||||||
any game outside the specified set. The orthogonal ``excludes``
|
any game outside the specified set.
|
||||||
channel is applied as a negative, consistent with every other
|
|
||||||
modifier. All other modifiers delegate to the criterion.
|
|
||||||
"""
|
|
||||||
if criterion.modifier in (Modifier.INCLUDES_ALL, Modifier.INCLUDES_ONLY) and criterion.value:
|
|
||||||
from games.models import Game, Purchase
|
|
||||||
|
|
||||||
|
``INCLUDES`` (plain "any") also uses a subquery instead of a raw
|
||||||
|
``games__in`` join because a single purchase linked to *n* of the
|
||||||
|
given games would appear *n* times in the result set (M2M join
|
||||||
|
duplicates).
|
||||||
|
|
||||||
|
The orthogonal ``excludes`` channel is applied as a negative,
|
||||||
|
consistent with every other modifier. All other modifiers delegate
|
||||||
|
to the criterion.
|
||||||
|
"""
|
||||||
|
# Empty value means no constraint; still apply excludes if any
|
||||||
|
if not criterion.value:
|
||||||
|
if criterion.excludes:
|
||||||
|
return ~Q(games__in=criterion.excludes)
|
||||||
|
return Q()
|
||||||
|
|
||||||
|
from games.models import Game, Purchase
|
||||||
|
|
||||||
|
if criterion.modifier in (Modifier.INCLUDES_ALL, Modifier.INCLUDES_ONLY):
|
||||||
subquery = Purchase.objects.all()
|
subquery = Purchase.objects.all()
|
||||||
for game_id in criterion.value:
|
for game_id in criterion.value:
|
||||||
subquery = subquery.filter(games=game_id)
|
subquery = subquery.filter(games=game_id)
|
||||||
@@ -417,6 +430,15 @@ class PurchaseFilter(OperatorFilter):
|
|||||||
if criterion.excludes:
|
if criterion.excludes:
|
||||||
q &= ~Q(games__in=criterion.excludes)
|
q &= ~Q(games__in=criterion.excludes)
|
||||||
return q
|
return q
|
||||||
|
|
||||||
|
if criterion.modifier == Modifier.INCLUDES:
|
||||||
|
# Use subquery to avoid duplicate rows from M2M join
|
||||||
|
subquery = Purchase.objects.filter(games__in=criterion.value)
|
||||||
|
q = Q(pk__in=subquery.values("pk"))
|
||||||
|
if criterion.excludes:
|
||||||
|
q &= ~Q(games__in=criterion.excludes)
|
||||||
|
return q
|
||||||
|
|
||||||
return criterion.to_q("games")
|
return criterion.to_q("games")
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -65,6 +65,60 @@
|
|||||||
if (noResults) noResults.classList.toggle("hidden", !visible);
|
if (noResults) noResults.classList.toggle("hidden", !visible);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ── Highlight tracking (filter mode) ──
|
||||||
|
var highlightedRow = null;
|
||||||
|
|
||||||
|
function highlightOption(row) {
|
||||||
|
clearHighlight();
|
||||||
|
if (!row) return;
|
||||||
|
row.style.backgroundColor = "var(--color-brand, rgba(59, 130, 246, 0.15))";
|
||||||
|
row.style.outline = "1px solid var(--color-brand, #3b82f6)";
|
||||||
|
highlightedRow = row;
|
||||||
|
row.scrollIntoView({ block: "nearest" });
|
||||||
|
}
|
||||||
|
|
||||||
|
function clearHighlight() {
|
||||||
|
if (highlightedRow) {
|
||||||
|
highlightedRow.style.backgroundColor = "";
|
||||||
|
highlightedRow.style.outline = "";
|
||||||
|
highlightedRow = null;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
function getVisibleOptions() {
|
||||||
|
var all = options.querySelectorAll("[data-search-select-option]");
|
||||||
|
return Array.prototype.filter.call(all, function (row) {
|
||||||
|
return row.style.display !== "none";
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
function autoHighlight(query) {
|
||||||
|
var visible = getVisibleOptions();
|
||||||
|
if (visible.length === 0) {
|
||||||
|
clearHighlight();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
var lower = query.toLowerCase();
|
||||||
|
// 1. Starts-with match
|
||||||
|
for (var i = 0; i < visible.length; i++) {
|
||||||
|
var label = (visible[i].getAttribute("data-label") || "").toLowerCase();
|
||||||
|
if (lower && label.startsWith(lower)) {
|
||||||
|
highlightOption(visible[i]);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
// 2. Substring match (fuzzy-lite)
|
||||||
|
for (var j = 0; j < visible.length; j++) {
|
||||||
|
var subLabel = (visible[j].getAttribute("data-label") || "").toLowerCase();
|
||||||
|
if (lower && subLabel.indexOf(lower) !== -1) {
|
||||||
|
highlightOption(visible[j]);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
// 3. Fallback: first visible option
|
||||||
|
highlightOption(visible[0]);
|
||||||
|
}
|
||||||
|
|
||||||
// ── Render server-fetched rows into the panel ──
|
// ── Render server-fetched rows into the panel ──
|
||||||
function renderRows(items) {
|
function renderRows(items) {
|
||||||
options.querySelectorAll("[data-search-select-option]").forEach(function (row) {
|
options.querySelectorAll("[data-search-select-option]").forEach(function (row) {
|
||||||
@@ -140,6 +194,7 @@
|
|||||||
renderRows(items);
|
renderRows(items);
|
||||||
// Re-apply the live query: the box may hold more text than was sent.
|
// Re-apply the live query: the box may hold more text than was sent.
|
||||||
setNoResults(filterRows(search.value.trim()) === 0);
|
setNoResults(filterRows(search.value.trim()) === 0);
|
||||||
|
if (isFilter) autoHighlight(search.value.trim());
|
||||||
})
|
})
|
||||||
.catch(function (error) {
|
.catch(function (error) {
|
||||||
if (error && error.name === "AbortError") return; // superseded
|
if (error && error.name === "AbortError") return; // superseded
|
||||||
@@ -166,6 +221,7 @@
|
|||||||
} else {
|
} else {
|
||||||
setNoResults(filterRows(query) === 0);
|
setNoResults(filterRows(query) === 0);
|
||||||
}
|
}
|
||||||
|
if (isFilter) autoHighlight(query);
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── Single-select combobox: the search box shows the committed label;
|
// ── Single-select combobox: the search box shows the committed label;
|
||||||
@@ -188,12 +244,15 @@
|
|||||||
// Show whatever is already loaded; the server decides no-results.
|
// Show whatever is already loaded; the server decides no-results.
|
||||||
filterRows(search.value.trim());
|
filterRows(search.value.trim());
|
||||||
setNoResults(false);
|
setNoResults(false);
|
||||||
|
if (isFilter) autoHighlight(search.value.trim());
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
setNoResults(filterRows(search.value.trim()) === 0);
|
setNoResults(filterRows(search.value.trim()) === 0);
|
||||||
|
if (isFilter) autoHighlight(search.value.trim());
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
search.addEventListener("input", function () {
|
search.addEventListener("input", function () {
|
||||||
|
clearHighlight();
|
||||||
if (!multi) container._searchSelectDirty = true;
|
if (!multi) container._searchSelectDirty = true;
|
||||||
runSearch();
|
runSearch();
|
||||||
});
|
});
|
||||||
@@ -215,6 +274,45 @@
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ── Keyboard navigation (filter mode) ──
|
||||||
|
search.addEventListener("keydown", function (event) {
|
||||||
|
if (!isFilter) return;
|
||||||
|
var key = event.key;
|
||||||
|
if (key === "ArrowDown" || key === "ArrowUp" || key === "Enter" || key === "Escape") {
|
||||||
|
var visible = getVisibleOptions();
|
||||||
|
if (visible.length === 0) {
|
||||||
|
if (key === "Escape") hidePanel();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (key === "ArrowDown") {
|
||||||
|
event.preventDefault();
|
||||||
|
showPanel();
|
||||||
|
var idx = highlightedRow ? visible.indexOf(highlightedRow) : -1;
|
||||||
|
var next = visible[(idx + 1) % visible.length];
|
||||||
|
highlightOption(next);
|
||||||
|
} else if (key === "ArrowUp") {
|
||||||
|
event.preventDefault();
|
||||||
|
showPanel();
|
||||||
|
var idx = highlightedRow ? visible.indexOf(highlightedRow) : -1;
|
||||||
|
var prev = visible[(idx - 1 + visible.length) % visible.length];
|
||||||
|
highlightOption(prev);
|
||||||
|
} else if (key === "Enter") {
|
||||||
|
if (highlightedRow) {
|
||||||
|
event.preventDefault();
|
||||||
|
var option = optionFromRow(highlightedRow);
|
||||||
|
addFilterPill(option, "include");
|
||||||
|
search.value = "";
|
||||||
|
clearHighlight();
|
||||||
|
hidePanel();
|
||||||
|
}
|
||||||
|
} else if (key === "Escape") {
|
||||||
|
clearHighlight();
|
||||||
|
hidePanel();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
// Clicking an option must not blur the input before the click selects.
|
// Clicking an option must not blur the input before the click selects.
|
||||||
options.addEventListener("mousedown", function (event) {
|
options.addEventListener("mousedown", function (event) {
|
||||||
event.preventDefault();
|
event.preventDefault();
|
||||||
@@ -243,10 +341,18 @@
|
|||||||
}
|
}
|
||||||
// Include / exclude button on a value row.
|
// Include / exclude button on a value row.
|
||||||
var button = event.target.closest("[data-search-select-action]");
|
var button = event.target.closest("[data-search-select-action]");
|
||||||
if (!button) return;
|
if (button) {
|
||||||
var row = button.closest("[data-search-select-option]");
|
var row = button.closest("[data-search-select-option]");
|
||||||
if (!row) return;
|
if (!row) return;
|
||||||
addFilterPill(optionFromRow(row), button.getAttribute("data-search-select-action"));
|
addFilterPill(optionFromRow(row), button.getAttribute("data-search-select-action"));
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
// Click on the option row itself → include.
|
||||||
|
var optionRow = event.target.closest("[data-search-select-option]");
|
||||||
|
if (optionRow) {
|
||||||
|
addFilterPill(optionFromRow(optionRow), "include");
|
||||||
|
return;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add (or re-type) an include/exclude pill for a value. Selecting any value
|
// Add (or re-type) an include/exclude pill for a value. Selecting any value
|
||||||
@@ -258,6 +364,7 @@
|
|||||||
);
|
);
|
||||||
if (existing) existing.remove();
|
if (existing) existing.remove();
|
||||||
pills.appendChild(buildFilterValuePill(option, kind));
|
pills.appendChild(buildFilterValuePill(option, kind));
|
||||||
|
search.value = "";
|
||||||
emitChange(null);
|
emitChange(null);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -334,6 +334,33 @@ class TestPurchaseGamesIncludesAllAgainstDB:
|
|||||||
result = set(Purchase.objects.filter(pf.to_q()))
|
result = set(Purchase.objects.filter(pf.to_q()))
|
||||||
assert result == {seeded["both"], seeded["only_a"], seeded["all_three"]}
|
assert result == {seeded["both"], seeded["only_a"], seeded["all_three"]}
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
def test_includes_any_no_duplicates(self):
|
||||||
|
"""INCLUDES [A, B] must not return duplicate rows for a purchase linked
|
||||||
|
to both A and B — the M2M join must not inflate the result.
|
||||||
|
|
||||||
|
Regression: ``games__in`` on a many-to-many field produces one row per
|
||||||
|
matching through-table entry, so a purchase linked to N of the selected
|
||||||
|
games would appear N times. The fix uses a subquery so each purchase
|
||||||
|
appears at most once.
|
||||||
|
"""
|
||||||
|
from games.filters import PurchaseFilter
|
||||||
|
from games.models import Purchase
|
||||||
|
|
||||||
|
seeded = self._seed()
|
||||||
|
pf = PurchaseFilter.from_json(
|
||||||
|
{
|
||||||
|
"games": {
|
||||||
|
"value": [seeded["a"].id, seeded["b"].id],
|
||||||
|
"modifier": "INCLUDES",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
)
|
||||||
|
result = list(Purchase.objects.filter(pf.to_q()))
|
||||||
|
# Must have 3 distinct purchases, not duplicates
|
||||||
|
assert len(result) == 3
|
||||||
|
assert set(result) == {seeded["both"], seeded["only_a"], seeded["all_three"]}
|
||||||
|
|
||||||
@pytest.mark.django_db
|
@pytest.mark.django_db
|
||||||
def test_includes_all_strips_embedded_labels(self):
|
def test_includes_all_strips_embedded_labels(self):
|
||||||
"""Stash-style {id, label} value items are normalised to bare ids."""
|
"""Stash-style {id, label} value items are normalised to bare ids."""
|
||||||
|
|||||||
Reference in New Issue
Block a user