Skip to content

Fix: Filter only active accounts in booking view#363

Merged
lg2de merged 10 commits intolg2de:mainfrom
shaig-mahmudov:main
Jan 6, 2026
Merged

Fix: Filter only active accounts in booking view#363
lg2de merged 10 commits intolg2de:mainfrom
shaig-mahmudov:main

Conversation

@shaig-mahmudov
Copy link
Copy Markdown
Contributor

@shaig-mahmudov shaig-mahmudov commented Jan 1, 2026

Related issue

Fixes #362

@lg2de
Copy link
Copy Markdown
Owner

lg2de commented Jan 1, 2026

Thanks for your support.
Please add an unit test validating the fixed behavior.

@lg2de lg2de added this to the 2.5 milestone Jan 1, 2026
@shaig-mahmudov
Copy link
Copy Markdown
Contributor Author

Update: I've added the unit test covering the active/inactive account filtering logic. Verified locally
Screenshot 2026-01-02 004326

@lg2de
Copy link
Copy Markdown
Owner

lg2de commented Jan 1, 2026

Looking deeper on your changes I'm a bit confused. Did you try to fix the bug I filled today? If so, you are working on the wrong view model. I was talking about the import, so ImportBookingViewModel needs to updated.
Your changes may be correct, but currently no real issue, because the list of accounts is already filtered when inserting. Currently I cannot confirm to see inactive accounts in the booking editor.

Also added a unit test to ensure only active accounts with valid mappings are shown.
@shaig-mahmudov
Copy link
Copy Markdown
Contributor Author

shaig-mahmudov commented Jan 1, 2026

Description: Fixed a bug where inactive accounts were appearing in the import list. I updated ImportBookingsViewModel to filter accounts by their Active status. I also reverted the accidental changes in BookingEditor.

Unit Tests: Added a new test case ImportAccounts_InactiveAccount_Excluded to verify the fix.

Screenshot 2026-01-02 024210

@lg2de
Copy link
Copy Markdown
Owner

lg2de commented Jan 2, 2026

Thanks for the update. The change looks correct, the test too. (I only will request some cosmetic changes separately.)
But, this does not fix the problem I faced. In my project I do have only one account with import configuration and this is active. But, I have several other accounts which are available as "remote account" which are inactive. These accounts should get filtered. (I've updated the bug to be more precise. I did not really expect that somebody else will take care... So, thanks a lot!)

Comment thread tests/SimpleAccounting.UnitTests/Presentation/ImportBookingsViewModelTests.cs Outdated
…ewModelTests.cs

Co-authored-by: Lukas Grützmacher <44983012+lg2de@users.noreply.github.com>
@shaig-mahmudov
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! I'm really enthusiastic about contributing to this project, so I'm glad I could help. I'll add the filtering for inactive accounts and update the code soon.

- Added null check for ImportMapping.Columns to prevent NullReferenceException.
- Improved filtering logic to ensure only active accounts with valid
  remote configurations are included in ImportAccounts.
- Updated unit tests to cover edge cases, including null and empty mappings.
@shaig-mahmudov
Copy link
Copy Markdown
Contributor Author

Hi, I've addressed your feedback regarding the inactive remote accounts:

Logic Change: Updated the ImportAccounts filter in ImportBookingsViewModel. I've added an explicit check for ImportMapping.Columns != null before calling IsValid(). This ensures that even if a mapping object exists, it is ignored if it doesn't contain the necessary column definitions.

Robustness: This change also prevents a potential NullReferenceException that could occur if the Columns list was null during the XML deserialization.

Unit Tests: I've expanded the unit tests to cover these scenarios:

Active account with empty columns (Remote Inactive).

Active account with null columns (Safety check).

Inactive account with valid mapping (Excluded).

All tests are passing. Ready for your review!

Screenshot 2026-01-04 202723

- Added null check for ImportMapping.Columns to prevent NullReferenceException.
- Improved filtering logic to ensure only active accounts with valid
  remote configurations are included in ImportAccounts.
- Updated unit tests to cover edge cases, including null and empty mappings.
@lg2de
Copy link
Copy Markdown
Owner

lg2de commented Jan 4, 2026

Sorry, but still you did not touch the list of remote accounts. So, "my bug" cannot be fixed.

The check on null columns is fine, but should be integration into IsValid.

Regarding the tests: Please always use exactly on empty line between "arrange", "act" and "assert", not more if somehow possible. Ensure consistent format regarding indentation.

@shaig-mahmudov
Copy link
Copy Markdown
Contributor Author

Thanks for the guidance. I've applied the cosmetic changes and refactored the unit tests as you requested. However, I’m still struggling to reproduce the exact issue with the remote accounts on my end. Could you please provide a quick example of the account configuration that is still causing the bug? I want to make sure I'm testing the right scenario before I submit the final fix.

@lg2de
Copy link
Copy Markdown
Owner

lg2de commented Jan 4, 2026

Account 100: Bank account - with import configuration
Account 600: Shoes - inactive
Account 601: New Shoes - active

The list of remote accounts (for all entries in the list) should only show "New Shoes".
Note, that you must add entries to LoadedData and check its property Accounts.

@shaig-mahmudov
Copy link
Copy Markdown
Contributor Author

Thanks for your patience. Refactored ImportEntryViewModel: Filtered the IEnumerable<AccountDefinition> accounts list in the constructor to only allow accounts where Active == true. Added Unit Test: Created LoadData_RemoteAccountList_ContainsOnlyActiveAccounts in ImportBookingsViewModel.

@lg2de lg2de merged commit ea693f9 into lg2de:main Jan 6, 2026
1 check failed
@lg2de
Copy link
Copy Markdown
Owner

lg2de commented Jan 6, 2026

Thanks!!

@shaig-mahmudov
Copy link
Copy Markdown
Contributor Author

You're welcome! I learned a lot from this process, especially regarding the data structure of remote accounts. Thanks for your patience and guidance along the way. Happy to contribute!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Booking import dialog should show active accounts only

2 participants