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

Fix category is/is not (nothing) filters #3669

Merged
merged 17 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const filterFields = [
'cleared',
'reconciled',
'saved',
'transfer',
].map(field => [field, mapField(field)]);

function ConfigureField({
Expand Down
3 changes: 2 additions & 1 deletion packages/loot-core/src/server/accounts/rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,11 @@ const CONDITION_TYPES = {
'isNot',
'doesNotContain',
'notOneOf',
'and',
],
nullable: true,
parse(op, value, fieldName) {
if (op === 'oneOf' || op === 'notOneOf') {
if (op === 'oneOf' || op === 'notOneOf' || op === 'and') {
assert(
Array.isArray(value),
'no-empty-array',
Expand Down
33 changes: 31 additions & 2 deletions packages/loot-core/src/server/accounts/transaction-rules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -372,21 +372,22 @@ describe('Transaction rules', () => {
await loadRules();
const account = await db.insertAccount({ name: 'bank' });
const categoryGroupId = await db.insertCategoryGroup({ name: 'general' });
const categoryId = await db.insertCategory({
const foodCategoryId = await db.insertCategory({
name: 'food',
cat_group: categoryGroupId,
});
const krogerId = await db.insertPayee({ name: 'kroger' });
const lowesId = await db.insertPayee({
name: 'lowes',
category: categoryId,
category: foodCategoryId,
});

await db.insertTransaction({
id: '1',
date: '2020-10-01',
account,
payee: krogerId,
category: foodCategoryId,
notes: 'barr',
amount: 353,
});
Expand Down Expand Up @@ -419,6 +420,7 @@ describe('Transaction rules', () => {
date: '2020-10-16',
account,
payee: lowesId,
category: foodCategoryId,
notes: '',
amount: 124,
});
Expand Down Expand Up @@ -492,8 +494,35 @@ describe('Transaction rules', () => {
]);
expect(transactions.map(t => t.id)).toEqual(['4', '5', '2', '3']);

//Condition special cases
//is category null
transactions = await getMatchingTransactions([
{ field: 'category', op: 'is', value: null },
]);
expect(transactions.map(t => t.id)).toEqual(['4', '2', '3']);

//category is not X
transactions = await getMatchingTransactions([
{ field: 'category', op: 'isNot', value: null },
]);
expect(transactions.map(t => t.id)).toEqual(['5', '1']);

// todo: isapprox
});

test('and sub expression builds $and condition', async () => {
const conds = [{ field: 'category', op: 'is', value: null }];
const { filters } = conditionsToAQL(conds);
expect(filters).toStrictEqual([
{
$and: [
{ category: { $eq: null } },
{ transfer_id: { $eq: null } },
{ is_parent: { $eq: false } },
],
},
]);
});
});

describe('Learning categories', () => {
Expand Down
58 changes: 55 additions & 3 deletions packages/loot-core/src/server/accounts/transaction-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,34 @@ export async function runRules(trans) {
return await finalizeTransactionForRules(finalTrans);
}

function conditionSpecialCases(cond: Condition): Condition {
//special cases that require multiple conditions
if (cond.op === 'is' && cond.field === 'category' && cond.value === null) {
return new Condition(
'and',
cond.field,
[
cond,
new Condition('is', 'transfer', false, null),
new Condition('is', 'parent', false, null),
],
{},
);
} else if (
cond.op === 'isNot' &&
cond.field === 'category' &&
cond.value === null
) {
return new Condition(
'and',
cond.field,
[cond, new Condition('is', 'parent', false, null)],
{},
);
}
return cond;
}

// This does the inverse: finds all the transactions matching a rule
export function conditionsToAQL(conditions, { recurDateBounds = 100 } = {}) {
const errors = [];
Expand All @@ -309,11 +337,13 @@ export function conditionsToAQL(conditions, { recurDateBounds = 100 } = {}) {
return null;
}
})
.map(conditionSpecialCases)
.filter(Boolean);

// rule -> actualql
const filters = conditions.map(cond => {
const { type, field, op, value, options } = cond;
const mapConditionToActualQL = cond => {
const { type, options } = cond;
let { field, op, value } = cond;

const getValue = value => {
if (type === 'number') {
Expand All @@ -322,6 +352,23 @@ export function conditionsToAQL(conditions, { recurDateBounds = 100 } = {}) {
return value;
};

if (field === 'transfer' && op === 'is') {
field = 'transfer_id';
if (value) {
op = 'isNot';
value = null;
} else {
value = null;
}
} else if (field === 'parent' && op === 'is') {
field = 'is_parent';
if (value) {
op = 'true';
} else {
op = 'false';
}
}

const apply = (field, op, value) => {
if (type === 'number') {
if (options) {
Expand Down Expand Up @@ -504,11 +551,16 @@ export function conditionsToAQL(conditions, { recurDateBounds = 100 } = {}) {
return apply(field, '$eq', true);
case 'false':
return apply(field, '$eq', false);
case 'and':
return {
$and: getValue(value).map(subExpr => mapConditionToActualQL(subExpr)),
};
default:
throw new Error('Unhandled operator: ' + op);
}
});
};

const filters = conditions.map(mapConditionToActualQL);
return { filters, errors };
}

Expand Down
12 changes: 12 additions & 0 deletions packages/loot-core/src/server/aql/compiler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -951,4 +951,16 @@ describe('Type conversions', () => {
);
expect(result.sql).toMatch('WHERE (transactions.payee IS NULL)');
});

it('allows fields to be not nullable', () => {
// With validated refs
const result = generateSQLWithState(
q('transactions')
.filter({ payee: { $ne: null } })
.select()
.serialize(),
schemaWithRefs,
);
expect(result.sql).toMatch('WHERE (payees1.id IS NOT NULL)');
});
});
2 changes: 1 addition & 1 deletion packages/loot-core/src/server/aql/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ const compileOp = saveStack('op', (state, fieldRef, opData) => {
}
case '$ne': {
if (castInput(state, rhs, lhs.type).type === 'null') {
return `${val(state, lhs)} IS NULL`;
UnderKoen marked this conversation as resolved.
Show resolved Hide resolved
return `${val(state, lhs)} IS NOT NULL`;
}

const [left, right] = valArray(state, [lhs, rhs], [null, lhs.type]);
Expand Down
19 changes: 13 additions & 6 deletions packages/loot-core/src/shared/rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ const TYPE_INFO = {

type FieldInfoConstraint = Record<
keyof FieldValueTypes,
{ type: keyof typeof TYPE_INFO; disallowedOps?: Set<RuleConditionOp> }
{
type: keyof typeof TYPE_INFO;
disallowedOps?: Set<RuleConditionOp>;
internalOps?: Set<RuleConditionOp>;
}
>;

const FIELD_INFO = {
Expand All @@ -67,11 +71,13 @@ const FIELD_INFO = {
date: { type: 'date' },
notes: { type: 'string' },
amount: { type: 'number' },
category: { type: 'id' },
category: { type: 'id', internalOps: new Set(['and']) },
account: { type: 'id' },
cleared: { type: 'boolean' },
reconciled: { type: 'boolean' },
saved: { type: 'saved' },
transfer: { type: 'boolean' },
parent: { type: 'boolean' },
} as const satisfies FieldInfoConstraint;

const fieldInfo: FieldInfoConstraint = FIELD_INFO;
Expand All @@ -85,11 +91,12 @@ export const FIELD_TYPES = new Map<keyof FieldValueTypes, string>(

export function isValidOp(field: keyof FieldValueTypes, op: RuleConditionOp) {
const type = FIELD_TYPES.get(field);
if (!type) {
return false;
}

if (!type) return false;
if (fieldInfo[field].disallowedOps?.has(op)) return false;

return (
TYPE_INFO[type].ops.includes(op) && !fieldInfo[field].disallowedOps?.has(op)
TYPE_INFO[type].ops.includes(op) || fieldInfo[field].internalOps?.has(op)
);
}

Expand Down
3 changes: 3 additions & 0 deletions packages/loot-core/src/types/models/rule.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export type RuleConditionOp =
| 'contains'
| 'doesNotContain'
| 'hasTags'
| 'and'
| 'matches';

type FieldValueTypes = {
Expand All @@ -38,6 +39,8 @@ type FieldValueTypes = {
payee_name: string;
imported_payee: string;
saved: string;
transfer: boolean;
parent: boolean;
cleared: boolean;
reconciled: boolean;
};
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/3669.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Bugfix
authors: [qedi-r]
---

Fix category filters when the value is '(nothing)'