Skip to content
Open
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
19 changes: 15 additions & 4 deletions server/graphql/common/expenses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4152,6 +4152,12 @@ export const moveExpenses = async (req: express.Request, expenses: Expense[], de
// -- Move expenses --
const expenseIds: number[] = uniq(expenses.map(expense => expense.id));
const recurringExpenseIds: number[] = uniq(expenses.map(expense => expense.RecurringExpenseId).filter(Boolean));

// Check if moving to a different host - if so, we need to clear accounting categories
const isMovingToDifferentHost = expenses.some(
e => e.collective.HostCollectiveId !== destinationAccount.HostCollectiveId,
);

const result = await sequelize.transaction(async dbTransaction => {
const associatedTransactionsCount = await models.Transaction.count({
where: { ExpenseId: expenseIds },
Expand All @@ -4162,9 +4168,12 @@ export const moveExpenses = async (req: express.Request, expenses: Expense[], de
throw new ValidationFailed('Cannot move expenses with associated transactions');
}

// Moving associated models
// Moving expenses - clear AccountingCategoryId if moving to different host (category belongs to old host)
const [, updatedExpenses] = await models.Expense.update(
{ CollectiveId: destinationAccount.id },
{
CollectiveId: destinationAccount.id,
...(isMovingToDifferentHost && { AccountingCategoryId: null }),
},
Comment on lines +4171 to +4176
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clear category per-expense when batch spans hosts

The new isMovingToDifferentHost flag is computed once for the whole batch, so if any expense in the list comes from a different host, the Expense.update clears AccountingCategoryId for all moved expenses. That means a mixed batch (some expenses already under the destination host) will lose valid accounting categories even though they remain within the same host. This is data loss that depends on moving multiple expenses at once from different hosts; consider clearing per-expense based on its original collective.HostCollectiveId instead of a batch-wide flag.

Useful? React with 👍 / 👎.

{
transaction: dbTransaction,
returning: true,
Expand Down Expand Up @@ -4231,7 +4240,7 @@ export const moveExpenses = async (req: express.Request, expenses: Expense[], de
},
);

// Record the migration log
// Record the migration log (previousExpenseValues includes AccountingCategoryId for audit)
await models.MigrationLog.create(
{
type: MigrationLogType.MOVE_EXPENSES,
Expand All @@ -4243,7 +4252,9 @@ export const moveExpenses = async (req: express.Request, expenses: Expense[], de
comments: updatedComments.map(c => c.id),
activities: updatedActivities.map(a => a.id),
destinationAccount: destinationAccount.id,
previousExpenseValues: mapValues(keyBy(expenses, 'id'), expense => pick(expense, ['CollectiveId'])),
previousExpenseValues: mapValues(keyBy(expenses, 'id'), expense =>
pick(expense, ['CollectiveId', 'AccountingCategoryId']),
),
},
},
{ transaction: dbTransaction },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,12 +253,12 @@ describe('server/graphql/v2/mutation/AccountingCategoriesMutations', () => {
expect(getNodesFromResult(result3)).to.have.length(0);

// Check activities
await sleep(100); // For the async activity creation
const activities = await models.Activity.findAll({
where: { CollectiveId: host.id, type: ActivityTypes.ACCOUNTING_CATEGORIES_EDITED },
order: [['createdAt', 'ASC']],
});

await sleep(100); // For the async activity creation
expect(activities).to.have.length(3);
activities.forEach(activity => {
expect(activity.HostCollectiveId).to.equal(host.id);
Expand Down
51 changes: 50 additions & 1 deletion test/server/graphql/v2/mutation/RootMutations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import roles from '../../../../../server/constants/roles';
import { TwoFactorAuthenticationHeader } from '../../../../../server/lib/two-factor-authentication/lib';
import models from '../../../../../server/models';
import {
fakeAccountingCategory,
fakeActivity,
fakeCollective,
fakeComment,
fakeExpense,
fakeHost,
fakeMember,
fakeRecurringExpense,
fakeTransaction,
Expand Down Expand Up @@ -171,7 +173,9 @@ describe('server/graphql/v2/mutation/RootMutations', () => {
expenses: [testExpense.id],
recurringExpenses: [],
destinationAccount: testCollective.id,
previousExpenseValues: { [testExpense.id]: { CollectiveId: previousCollective.id } },
previousExpenseValues: {
[testExpense.id]: { CollectiveId: previousCollective.id, AccountingCategoryId: null },
},
activities: [],
comments: [],
});
Expand Down Expand Up @@ -263,6 +267,51 @@ describe('server/graphql/v2/mutation/RootMutations', () => {
await recurringExpense.reload();
expect(recurringExpense.CollectiveId).to.eq(collective.id);
});

it('clears accounting category when moving expense to a different host', async () => {
const sourceHost = await fakeHost();
const sourceCollective = await fakeCollective({ HostCollectiveId: sourceHost.id });
const accountingCategory = await fakeAccountingCategory({ CollectiveId: sourceHost.id, kind: 'EXPENSE' });
const destinationHost = await fakeHost();
const destinationCollective = await fakeCollective({ HostCollectiveId: destinationHost.id });

const expense = await fakeExpense({
CollectiveId: sourceCollective.id,
AccountingCategoryId: accountingCategory.id,
});

const result = await callMoveExpenseMutation(
{ destinationAccount: { legacyId: destinationCollective.id }, expenses: [{ legacyId: expense.id }] },
rootUser,
);

result.errors && console.error(result.errors);
expect(result.errors).to.not.exist;
await expense.reload();
expect(expense.AccountingCategoryId).to.be.null;
});

it('preserves accounting category when moving expense within the same host', async () => {
const host = await fakeHost();
const sourceCollective = await fakeCollective({ HostCollectiveId: host.id });
const destinationCollective = await fakeCollective({ HostCollectiveId: host.id });
const accountingCategory = await fakeAccountingCategory({ CollectiveId: host.id, kind: 'EXPENSE' });

const expense = await fakeExpense({
CollectiveId: sourceCollective.id,
AccountingCategoryId: accountingCategory.id,
});

const result = await callMoveExpenseMutation(
{ destinationAccount: { legacyId: destinationCollective.id }, expenses: [{ legacyId: expense.id }] },
rootUser,
);

result.errors && console.error(result.errors);
expect(result.errors).to.not.exist;
await expense.reload();
expect(expense.AccountingCategoryId).to.equal(accountingCategory.id);
});
});

describe('editAccountType', () => {
Expand Down
Loading