chore: remove is_sync_after_timestamp_enabled#890
Conversation
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughStandardizes QuickBooks Online entity syncing to a single per-entity sync_after timestamp, removes WorkspaceGeneralSettings gating and batching/transactional deactivation, always advances sync timestamps after each entity, removes the is_sync_after_timestamp_enabled field, updates fixtures/serializers, and expands tests to multi-entity sync flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Scheduler as Sync Trigger
participant Utils as QBO Utils
participant Store as SyncTimestamp Store
participant QBO as QBOSDK API
participant Dest as DestinationAttribute
participant TS as QBOSyncTimestamp
Scheduler->>Utils: sync(entity_type, workspace_id)
Utils->>Store: get_entity_sync_timestamp(workspace_id, entity_type)
Store-->>Utils: sync_after (str or None)
Utils->>QBO: get_all_generator(sync_after)
QBO-->>Utils: current records (iter)
Utils->>QBO: get_inactive(sync_after)
QBO-->>Utils: inactive records (iter)
Utils->>Dest: bulk_create_or_update_destination_attributes(current + inactive)
Utils->>TS: update_sync_timestamp(workspace_id, entity_type)
TS-->>Utils: ack
Utils-->>Scheduler: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)tests/test_quickbooks_online/test_utils.py (2)
🔇 Additional comments (11)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Diff Coverage
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/quickbooks_online/utils.py (1)
62-71: Return type annotation mismatch detected.The function is annotated to return
strbut can actually returnNonewhengetattr(qbo_sync_timestamp, f'{entity_type}_synced_at')isNone.-def get_entity_sync_timestamp(workspace_id: int, entity_type: str) -> str: +def get_entity_sync_timestamp(workspace_id: int, entity_type: str) -> Optional[str]:Also add the import:
-from typing import Dict, List, Optional +from typing import Dict, List, Optional
♻️ Duplicate comments (1)
tests/test_fyle_integrations_imports/test_modules/test_tax_groups.py (1)
172-179: Code duplication in subsequent test case.The same mock patches are repeated again. This reinforces the need for a shared fixture.
🧹 Nitpick comments (3)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
5-5: Avoid committing environment-specific dump headers to reduce diff churnThe pgdg build metadata in the header tends to change across machines/containers and creates noisy diffs with no functional impact. Consider normalizing or stripping pg_dump header lines in fixtures generation.
tests/test_fyle_integrations_imports/test_modules/test_tax_groups.py (1)
123-130: Duplicate mock patches should be optimized.The same patches are repeated across multiple test cases. Consider extracting these common mocks into a test fixture to reduce code duplication and improve maintainability.
+@pytest.fixture +def mock_tax_code_sync(mocker): + """Common mocks for tax code sync operations.""" + mocker.patch('qbosdk.apis.TaxCodes.get_inactive', return_value=[]) + mocker.patch('apps.quickbooks_online.utils.get_entity_sync_timestamp', return_value=None) + def test_auto_create_destination_attributes(mocker, db): # ... existing code ... - mocker.patch( - 'qbosdk.apis.TaxCodes.get_inactive', - return_value=[] - ) - mocker.patch( - 'apps.quickbooks_online.utils.get_entity_sync_timestamp', - return_value=None - )tests/test_quickbooks_online/test_utils.py (1)
76-117: Consider extracting common test patterns.The sync tests for accounts, employees, and vendors follow an identical pattern. Consider creating a parameterized test or helper function to reduce duplication.
@pytest.mark.parametrize("entity_config", [ { 'entity_type': 'account', 'api_module': 'Accounts', 'attribute_type': 'ACCOUNT', 'workspace_id': 1, 'min_count': 60, }, { 'entity_type': 'employee', 'api_module': 'Employees', 'attribute_type': 'EMPLOYEE', 'workspace_id': 3, 'min_count': 2, }, # ... other entities ]) def test_sync_entity_with_sync_after(mocker, db, entity_config): """Test sync functionality with sync_after for various entities.""" # Common test logic hereAlso applies to: 119-161, 163-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/quickbooks_online/utils.py(21 hunks)apps/workspaces/migrations/0056_remove_workspacegeneralsettings_is_sync_after_timestamp_enabled.py(1 hunks)apps/workspaces/models.py(0 hunks)apps/workspaces/serializers.py(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(6 hunks)tests/test_fyle_integrations_imports/test_modules/test_tax_groups.py(4 hunks)tests/test_quickbooks_online/test_utils.py(1 hunks)
💤 Files with no reviewable changes (1)
- apps/workspaces/models.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_quickbooks_online/test_utils.py (3)
apps/workspaces/models.py (2)
QBOCredential(144-165)get_active_qbo_credentials(164-165)apps/quickbooks_online/utils.py (10)
QBOConnector(108-1696)sync_items(253-298)sync_accounts(300-429)sync_employees(617-644)sync_vendors(532-580)sync_departments(431-476)sync_classes(646-708)sync_customers(710-754)sync_tax_codes(478-530)get_or_create_vendor(184-209)apps/quickbooks_online/models.py (1)
QBOSyncTimestamp(1052-1086)
apps/quickbooks_online/utils.py (2)
apps/quickbooks_online/models.py (2)
QBOSyncTimestamp(1052-1086)update_sync_timestamp(1077-1086)apps/workspaces/helpers.py (1)
get_app_name(26-31)
🪛 Pylint (3.3.7)
apps/workspaces/migrations/0056_remove_workspacegeneralsettings_is_sync_after_timestamp_enabled.py
[error] 1-1: Unrecognized option found: no-space-check
(E0015)
[refactor] 1-1: Useless option value for '--disable', 'print-statement' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'parameter-unpacking' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'unpacking-in-except' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'old-raise-syntax' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'backtick' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'import-star-module-level' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'apply-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'basestring-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'buffer-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'cmp-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'coerce-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'execfile-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'file-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'long-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'raw_input-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'reduce-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'standarderror-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'unicode-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'xrange-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'coerce-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'delslice-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'getslice-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'setslice-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'no-absolute-import' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'old-division' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-iter-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-view-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'next-method-called' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'metaclass-assignment' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'indexing-exception' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'raising-string' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'reload-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'oct-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'hex-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'nonzero-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'cmp-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'input-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'round-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'intern-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'unichr-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'map-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'zip-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'range-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'filter-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'using-cmp-argument' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'div-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'idiv-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'rdiv-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'exception-message-attribute' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'invalid-str-codec' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'sys-max-int' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'bad-python3-import' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-string-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-str-translate-call' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-itertools-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-types-field' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'next-method-defined' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-items-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-keys-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-values-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-operator-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-urllib-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'xreadlines-attribute' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-sys-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'exception-escape' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'comprehension-escape' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'no-self-use' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers.
(R0022)
🔇 Additional comments (16)
apps/workspaces/serializers.py (1)
41-43: No lingering references tois_sync_after_timestamp_enabledfound — safe to approveVerified via a recursive search (
rg -n -S) that the field isn’t referenced anywhere outside of migrations or SQL fixtures. Removing it from the serializer’sexcludelist now correctly aligns with the model migration.apps/workspaces/migrations/0056_remove_workspacegeneralsettings_is_sync_after_timestamp_enabled.py (1)
12-17: Migration is minimal and correct for column removalThe RemoveField operation and dependency on 0055 look correct for dropping the column. No additional state ops needed given a simple boolean removal.
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (5)
1019-1020: WorkspaceGeneralSettings schema update looks correctColumn
is_sync_after_timestamp_enabledis removed and the table now ends withskip_accounting_export_summary_post boolean NOT NULL. No dangling comma or type mismatch in this snippet.
5738-5739: Migration entry added for column removal — LGTM
0056_remove_workspacegeneralsettings_is_sync_after_timestamp_enabledis recorded with id 238, consistent with the PR intent. Ensure the migration file exists and runs before tests that load this fixture.
35707-35708: Seq setval for django_migrations updated — ensure it matches max idSet to 238, which aligns with the added migration entry. If you run the verification script above, it confirms the setval equals the max id to prevent fixture load issues.
35173-35185: last_export_details sequence setval confirmedVerified that tests/sql_fixtures/reset_db_fixtures/reset_db.sql contains
SELECT pg_catalog.setval('public.last_export_details_id_seq', 5, true);at line 35840, so test inserts won’t collide. No further changes required.
35600-35606: Fixtures validated: COPY blocks align and omitted column confirmedBoth
public.workspace_general_settingsandpublic.last_export_detailsCOPY definitions now:
- Match their column counts against every data row (30 and 11 fields respectively).
- Correctly omit the
is_sync_after_timestamp_enabledcolumn.Occurrences of the string
is_sync_after_timestamp_enabledonly appear in thedjango_migrationsfixture as migration names (e.g.0054_workspacegeneralsettings_is_sync_after_timestamp_enabled), which is expected. No further changes are required.tests/test_fyle_integrations_imports/test_modules/test_tax_groups.py (2)
23-30: LGTM! The new mocks correctly support the per-entity sync timestamp mechanism.The addition of mocks for
get_inactiveandget_entity_sync_timestampaligns with the PR's objective to standardize entity syncing using per-entity timestamps. The mock returns are appropriate for initial sync scenarios.
152-152: Test assertion change requires clarification.The change from exact equality (
== 4) to minimum bound (>= 3) reduces test precision. This could mask potential issues if the actual count unexpectedly decreases.Could you explain why the exact count assertion was relaxed? If this is due to test environment variability, consider using a more deterministic test setup instead.
apps/quickbooks_online/utils.py (5)
262-262: Consistent pattern implementation for entity syncing.The implementation correctly follows a consistent pattern across all sync methods: get timestamp, fetch active records, fetch inactive records, then update timestamp. This aligns well with the PR's objective to standardize syncing behavior.
Also applies to: 281-281, 296-296
455-473: Good addition of inactive department handling.The new code properly handles inactive departments by stripping the "(deleted)" suffix and setting
active=False. This ensures consistency with other entity types' inactive handling.
510-527: Proper implementation of inactive tax code syncing.The inactive tax codes are correctly processed with tax rate calculation and proper value formatting. The implementation maintains consistency with the active tax code processing logic.
632-641: Consistent inactive entity handling across all resource types.The inactive employee processing follows the established pattern, properly handling display name cleanup and bulk attribute updates.
682-706: Complete implementation of inactive classes syncing.The inactive classes are properly handled with appropriate display name cleanup and bulk creation/update operations. The pattern is consistent with other entity types.
tests/test_quickbooks_online/test_utils.py (2)
30-74: Comprehensive test coverage for items sync with three distinct cases.The test properly validates all three sync scenarios (initial, with timestamp, and inactive) and correctly asserts the expected mock calls and database state. The implementation aligns with the new per-entity sync timestamp approach.
385-396: New vendor creation test validates the sync changes.The test properly validates vendor creation flow and the returned vendor ID, which is important for the overall sync functionality.
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
3 participants
Description
chore: remove is_sync_after_timestamp_enabled
Clickup
https://app.clickup.com/
Summary by CodeRabbit
Bug Fixes
Refactor
Chores