Feature/add tests and exceptions #84
Conversation
Summary of ChangesHello @Henderalex, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's stability and maintainability by bolstering its testing framework and improving error management. It introduces a detailed guide for developers, expands unit test coverage for critical components, and refines database interaction logic with custom exceptions and more precise data filtering. These changes collectively aim to make the codebase more reliable and easier to develop against. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant step forward, adding a comprehensive test suite and improving error handling with custom exceptions. The tests are well-structured and cover many important cases. The introduction of custom exceptions in the records manager is a great improvement for API clarity.
However, I've identified a critical issue in src/bagels/managers/records.py concerning database session management, which compromises transactional atomicity and could lead to data inconsistencies. There are also a few other high-severity logical errors in the same file related to split handling. I've provided detailed feedback and suggestions to address these points, which will help make the data layer more robust and reliable.
| def create_record_and_splits(record_data: dict, splits_data: list[dict]): | ||
| """Create a record and associated splits. | ||
|
|
||
| Raises: | ||
| InvalidRecordDataException: If inputs are of incorrect types. | ||
| DatabaseOperationException: If any DB operation fails. | ||
| """ | ||
| if not isinstance(splits_data, list): | ||
| raise InvalidRecordDataException("splits_data must be a list of mappings") | ||
|
|
||
| session = Session() | ||
| try: | ||
| record = create_record(record_data) | ||
| for split in splits_data: | ||
| if not isinstance(split, dict): | ||
| raise InvalidRecordDataException("each split must be a dict") | ||
| split["recordId"] = record.id | ||
| create_split(split) | ||
| return record | ||
| except SQLAlchemyError as e: | ||
| raise DatabaseOperationException("Failed to create record and splits") from e | ||
| finally: | ||
| session.close() |
There was a problem hiding this comment.
This function and update_record_and_splits are not transactionally atomic. create_record and create_split each manage their own database sessions and transactions. If create_split fails for a subsequent split, the record and any previous splits will have already been committed, leaving the database in an inconsistent state. The entire operation should succeed or fail together.
To fix this, the manager functions (create_record, create_split, etc.) should be refactored to accept an optional session argument. This function would then create a single session, pass it to the underlying calls, and commit the transaction only after all operations have succeeded.
| splits = get_splits_by_record_id(record_id) | ||
| return sum(split.amount for split in splits) | ||
| if splits is None: | ||
| raise RecordNotFoundException(f"No splits found for record id={record_id}") | ||
| return sum((getattr(split, "amount", 0) or 0) for split in splits) |
There was a problem hiding this comment.
The check if splits is None: is incorrect because get_splits_by_record_id always returns a list, which may be empty but will never be None. This makes the check dead code. Furthermore, raising a RecordNotFoundException is semantically incorrect here, as a record can validly exist without any splits. The original implementation correctly handled this by summing an empty list to 0.
The getattr and or 0 are also overly defensive, as split.amount is a non-nullable field on the Split model.
splits = get_splits_by_record_id(record_id)
return sum(split.amount for split in splits)| splits = get_splits_by_record_id(record_id) | ||
| return all(split.isPaid for split in splits) | ||
| if splits is None: | ||
| raise RecordNotFoundException(f"No splits found for record id={record_id}") | ||
| return all(getattr(split, "isPaid", False) for split in splits) |
There was a problem hiding this comment.
This function has the same logical flaw as get_record_total_split_amount. The check if splits is None: is incorrect because get_splits_by_record_id always returns a list. Raising RecordNotFoundException is also semantically wrong, as a record can exist without splits. For a record with no splits, all([]) correctly evaluates to True, which is the desired behavior. The original implementation was correct.
splits = get_splits_by_record_id(record_id)
return all(split.isPaid for split in splits)| "freezegun>=1.5.1", | ||
| "pytest-mock>=3.14.0", |
There was a problem hiding this comment.
The freezegun library is being added, but time-machine is already included as a dependency on line 77. Both libraries serve the same purpose of mocking time in tests. Including both adds unnecessary bloat to the project's dependencies and can cause confusion for developers. It's best practice to choose one and use it consistently.
| try: | ||
| operator, amount = get_operator_amount(operator_amount) | ||
| except Exception as e: | ||
| raise InvalidRecordDataException( | ||
| f"Invalid operator_amount format: {operator_amount}" | ||
| ) from e |
There was a problem hiding this comment.
Catching the generic Exception is too broad and can mask unexpected errors. The get_operator_amount function is most likely to raise a ValueError if the amount part of the string cannot be converted to a float. It's better to catch a more specific exception to make error handling more precise and avoid accidentally catching unrelated exceptions.
except ValueError as e:
raise InvalidRecordDataException(
f"Invalid operator_amount format: {operator_amount}"
) from e| assert getattr(m, "fromAccount", None) == template.accountId or str(getattr(m, "fromAccount", None)) == str(template.accountId) | ||
| assert getattr(m, "toAccount", None) == template.transferToAccountId or str(getattr(m, "toAccount", None)) == str(template.transferToAccountId) No newline at end of file |
There was a problem hiding this comment.
This lenient assertion (... or str(...) == str(...)) correctly identifies a type inconsistency in the TransferModal class. The fromAccount and toAccount attributes can be either an int or a str depending on the code path that sets them. This can lead to subtle bugs.
To improve robustness, the underlying issue in src/bagels/modals/transfer.py should be fixed to ensure these attributes are always integers (e.g., by casting the ID to int in on_list_view_highlighted). After that, this assertion can be simplified to a direct integer comparison.
2 participants
Team Member Names:
Alexandre Henderson
Tyler Smith
Summary of Changes:
Add freezegun to dev test dependencies
Tyler0724 committed 2 weeks ago
Exclude soft-deleted categories from counts
Tyler0724 committed 2 weeks ago
Add CRUD coverage tests for split manager
Tyler0724 committed 2 weeks ago
Add unit tests for records table builder and people module
Tyler0724 committed 2 weeks ago
Document testing workflow and provide feature config
Tyler0724 committed 2 weeks ago
Add pytest-mock to test dependencies
Tyler0724 committed 2 weeks ago
Expand unit tests with parametrization and mocks
Tyler0724 committed 2 weeks ago
Document testing workflow and contributor tips
Tyler0724 committed 2 weeks ago
Commits on Oct 30, 2025
Harden split manager tests with config setup and error coverage
Tyler0724 committed last week
Commits on Nov 3, 2025
Add Exception Handling to records.py
Henderalex committed last week
Create Tests for transfer.py
Henderalex committed last week
Created test for provider.py
Files Modified:
src/bagels/managers/splits.py
src/bagels/components/modules/records/_table_builder.py
src/bagels/components/modules/people.py
src/bagels/modals/transfer.py
src/bagels/provider.py
src/bagels/managers/records.py
pyproject.toml
Files Created:
TESTING.md
tests/test_provider.py
tests/modals/test_transfer
tests/managers/test_splits.py
tests/components/modules/records/test_table_builder.py
test/components/modals/test_people.py
Test Coverage Statistics
tests/test_provider.py: 100%
tests/modals/test_transfer: 90%
tests/managers/test_splits.py: 90%
tests/components/modules/records/test_table_builder.py: 100%
test/components/modals/test_people.py: 95%
Link to TESTING.md