Clear accounting category when moving to different host#11389
Clear accounting category when moving to different host#11389
Conversation
znarf
force-pushed
the
fix/move-expenses-clear-accounting-category
branch
from
14bf281 to
9e204be
Compare
… host When expenses are moved to an account under a different host, the AccountingCategoryId is now cleared since accounting categories belong to specific hosts and would be invalid after the move. The migration log now also captures the previous AccountingCategoryId for audit purposes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
znarf
force-pushed
the
fix/move-expenses-clear-accounting-category
branch
from
9e204be to
3f7b4f0
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f7b4f03b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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 }), | ||
| }, |
There was a problem hiding this comment.
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 👍 / 👎.
Betree
left a comment
Betree
left a comment
There was a problem hiding this comment.
This is a good change, but it doesn't cover the case we identified with manyverse/java-jda/material-table-core which all share the same story: they moved from a different host (or self hosted) to OSC. OSC then added the accounting category.
moveExpense was not the source of the problem in this case.
2 participants
When expenses are moved to an account under a different host, the AccountingCategoryId is now cleared since accounting categories belong to specific hosts and would be invalid after the move.
The migration log now also captures the previous AccountingCategoryId for audit purposes.