feat: add support for intacct attribute count api#842
Conversation
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughAdds a per-workspace SageIntacctAttributesCount model and migration, backfills rows via an internal RunSQL migration, initializes records on workspace creation, exposes a RetrieveAPIView and URL for counts, refactors sync gating to unified scalar limits with persisted per-attribute counts (including UDD), updates SQL fixtures, expands tests, and bumps sageintacctsdk. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as DRF: SageIntacctAttributesCountView
participant DB as DB: sage_intacct_attributes_count
Client->>API: GET /sage-intacct/attributes_count/?workspace_id={id}
API->>DB: SELECT * FROM sage_intacct_attributes_count WHERE workspace_id={id}
DB-->>API: AttributesCount row (or 404)
API-->>Client: 200 OK (serialized counts) / 404 Not Found
sequenceDiagram
autonumber
participant Sync as SyncWorker (utils)
participant Intacct as Sage Intacct API
participant Model as SageIntacctAttributesCount
participant DB as DB
rect rgba(230,245,230,0.6)
note right of Sync: Per-attribute flow: count → persist → gate → process/skip
Sync->>Intacct: Fetch attribute count (e.g., items)
Intacct-->>Sync: count = N
Sync->>Model: update_attribute_count(workspace_id, attribute_type, N)
Model->>DB: INSERT/UPDATE count & UDD details
Sync->>Sync: is_sync_allowed(N, attribute_type)
alt allowed
Sync->>Intacct: Fetch paginated records
Intacct-->>Sync: Records
Sync->>DB: Upsert records
else over limit
Sync-->>Sync: Log and skip processing
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/sage_intacct/utils.py (1)
228-243: Handle.count()failures and centralize count + gating behaviorThis codebase calls
.count()on various Sage Intacct resources (accounts, departments, expense_types, charge_card_accounts, payment_accounts, cost_types, cost_codes, projects, items, locations, expense_payment_types, employees, allocations, dimension_values, classes, customers, tax_details, vendors) without error handling. WrongParamsError and NoPrivilegeError are legitimate exceptions from the Intacct SDK—already used throughout the codebase (tasks.py, dependent_fields.py, etc.)—but they are not caught around the 18+.count()calls in utils.py. When.count()fails due to permission restrictions or unsupported filters, the entire sync breaks instead of being handled gracefully.The pattern is consistent across sync methods:
- Call
.count(...)- Persist via
SageIntacctAttributesCount.update_attribute_count(...)- Gate via
is_sync_allowed(...)and potentially return earlyConsider extracting a helper that wraps
count, persistence, and gating with controlled exception handling. First, add NoPrivilegeError to the import line (line 16):from sageintacctsdk.exceptions import WrongParamsError, NoPrivilegeErrorThen implement the helper:
def _get_and_record_count( self, attribute_type: str, *, is_cost_type: bool = False, **count_kwargs: Any, ) -> int | None: try: attribute_count = self.connection.__getattribute__(attribute_type).count(**count_kwargs) except (WrongParamsError, NoPrivilegeError) as exc: logger.info( "Skipping %s count for workspace %s due to Intacct error: %s", attribute_type, self.workspace_id, exc, ) return None SageIntacctAttributesCount.update_attribute_count( workspace_id=self.workspace_id, attribute_type=attribute_type, count=attribute_count, ) limit_type = "cost_types" if is_cost_type else None if not self.is_sync_allowed(attribute_count=attribute_count, attribute_type=limit_type): logger.info( "Skipping sync of %s for workspace %s as it has %s counts which is over the limit of %s", attribute_type, self.workspace_id, attribute_count, COST_TYPES_LIMIT if is_cost_type else SYNC_UPPER_LIMIT, ) return None return attribute_countThen each
sync_*method can early-return if this helper returnsNone, avoiding duplicated patterns and making error handling uniform.apps/sage_intacct/views.py (1)
248-316: Add timeout protection and resetis_expiredflag in OAuth credential flow.The OAuth flow has two issues that should be addressed:
Add timeout and JSON validation to
requests.post
The external call tosettings.INTACCT_TOKEN_URIat line 305 lacks a timeout and assumes a well-formed response with arefresh_tokenkey. This can cause hanging requests or 500 errors on malformed responses.Reset
is_expiredflag when updating credentials
Theupdate_or_createcall at lines 278–283 omitsis_expiredfrom defaults. If previous authentication attempts marked credentials as expired, the flag remainsTrueand subsequent calls toget_active_sage_intacct_credentials(which filtersis_expired=Falseatapps/workspaces/models.py:199) will fail. The old password-based auth flow resets this flag explicitly (seeapps/workspaces/views.py:376), and the new OAuth flow should do the same for consistency.Apply the suggested diffs from the original review, ensuring both the timeout protection and
is_expired: Falseaddition are included in theupdate_or_createcall.
🧹 Nitpick comments (7)
apps/workspaces/views.py (1)
26-38: Consider backfillingSageIntacctAttributesCountfor existing workspacesCreating a
SageIntacctAttributesCountrow alongside new workspaces is good, and aligns with the OneToOne link. However, older workspaces created before this change will only get a row lazily viaupdate_attribute_count(...).get_or_create(...)paths during sync.If the new
attributes_countAPI is expected to work immediately for all existing workspaces (even before any sync runs), you may want a data migration or management command to backfillSageIntacctAttributesCountrows for those workspaces.Also applies to: 123-131
apps/sage_intacct/utils.py (1)
179-195: Fixis_sync_allowedtype hint to resolve RUF013 lint warningThe parameter
attribute_typehas an invalid type hint. It declaresstrbut defaults toNone, which violates the RUF013 lint rule:def is_sync_allowed(self, attribute_count: int, attribute_type: str = None) -> bool:Update to make the optionality explicit:
- def is_sync_allowed(self, attribute_count: int, attribute_type: str = None) -> bool: + def is_sync_allowed(self, attribute_count: int, attribute_type: str | None = None) -> bool:All 16 call sites are compatible: they either pass no
attribute_typeargument or pass a string literal ('cost_types'). Python 3.10+ is already in use (Dockerfile specifiespython:3.10-slim), so PEP 604 syntax is supported.apps/sage_intacct/views.py (2)
90-143: SageIntacctFieldsView logic looks sound; minor note on queryset usage.The exclusion logic using chained
~Q(...)filters and distinctattribute_type/display_namepairs is consistent, and the override withDimensionDetailplus the special handling forALLOCATIONandPROJECTmatches the surrounding domain model.The only nit is that
dimensions.filter(...).exists()/.get(...)is called per attribute, which is fine for expected small cardinalities but could be optimized later (e.g., by pre-building a{attribute_type: display_name}dict) if needed.
318-333: JWT decode: acceptable for parsing, but consider proper verification.Decoding the Intacct refresh token with
verify_signature=Falseis sufficient to parse the payload and enforce a company‑ID consistency check, but it provides no assurance that the token was actually issued by Intacct (or that it hasn’t expired/tampered).If Intacct exposes the appropriate public keys or verification endpoint, consider moving towards proper verification (signature and expiry) when possible. If not, a brief code comment explaining why verification is disabled (e.g., “Intacct does not expose JWT verification keys; this decode is used only for cross‑checking cnyId against stored credentials”) would help future maintainers.
apps/sage_intacct/models.py (1)
2002-2050: SageIntacctAttributesCount model looks good; add safety around dynamic field updates.The model structure (OneToOne with
Workspace, per‑dimension*_countfields, anduser_defined_dimensions_detailsJSON) is consistent with the new API and should work well for per‑workspace limits and reporting.The
update_attribute_counthelper is convenient, but as implemented it assumesattribute_typealways maps to an existing*_countfield. If an unsupportedattribute_typeis ever passed (e.g., a typo or a newly added dimension without a corresponding field),save(update_fields=[field_name, ...])will raise aFieldError.To make this safer and easier to debug, consider guarding the dynamic update:
@staticmethod def update_attribute_count(workspace_id: int, attribute_type: str, count: int) -> None: """ Update attribute count for a workspace """ sage_intacct_count, _ = SageIntacctAttributesCount.objects.get_or_create( workspace_id=workspace_id ) field_name = f'{attribute_type}_count' - setattr(sage_intacct_count, field_name, count) - sage_intacct_count.save(update_fields=[field_name, 'updated_at']) + if not hasattr(sage_intacct_count, field_name): + # Optional: log instead of raising, depending on how strict you want this to be + raise ValueError(f'Unsupported attribute_type "{attribute_type}" for SageIntacctAttributesCount') + + setattr(sage_intacct_count, field_name, count) + sage_intacct_count.save(update_fields=[field_name, 'updated_at'])tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
11992-11992: Verify necessity ofupdated_atindex.An index on
updated_atis created for sorting/filtering by recency. Confirm this index is actually used by the API endpoint queries; if not, consider removing it to reduce maintenance and storage overhead.apps/sage_intacct/migrations/0032_sageintacctattributescount.py (1)
18-40: Schema is sound; consider tightening count types and JSONField semanticsOverall this migration looks consistent and safe to apply. A couple of small, non‑blocking tweaks you might want to consider:
- For all
*_countfields (Lines 19–31, 33–36), usingmodels.PositiveIntegerFieldinstead ofmodels.IntegerFieldwould better reflect that these are non‑negative counters and prevent negative values from being persisted by mistake.- For
user_defined_dimensions_details(Line 32), you currently allow bothnulland{}as “empty” states (null=Trueplusdefault=dict). If you don’t need to distinguish between “no data yet” and “no dimensions”, you might simplify by droppingnull=Trueand just using{}as the canonical empty value.If you decide to adopt this, the migration snippet could look like:
- ('accounts_count', models.IntegerField(default=0, help_text='Number of accounts in Sage Intacct')), + ('accounts_count', models.PositiveIntegerField(default=0, help_text='Number of accounts in Sage Intacct')), @@ - ('allocations_count', models.IntegerField(default=0, help_text='Number of allocations in Sage Intacct')), + ('allocations_count', models.PositiveIntegerField(default=0, help_text='Number of allocations in Sage Intacct')), @@ - ('user_defined_dimensions_details', models.JSONField(blank=True, default=dict, help_text='Count breakdown per UDD dimension (e.g., {"DEPARTMENT": 150, "PROJECT": 500}). Not exposed via API.', null=True)), + ('user_defined_dimensions_details', models.JSONField(blank=True, default=dict, help_text='Count breakdown per UDD dimension (e.g., {"DEPARTMENT": 150, "PROJECT": 500}). Not exposed via API.', null=False)),(And mirror the same types in the corresponding model definition.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/sage_intacct/migrations/0032_sageintacctattributescount.py(1 hunks)apps/sage_intacct/models.py(2 hunks)apps/sage_intacct/serializers.py(2 hunks)apps/sage_intacct/urls.py(1 hunks)apps/sage_intacct/utils.py(23 hunks)apps/sage_intacct/views.py(2 hunks)apps/workspaces/views.py(2 hunks)requirements.txt(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/workspaces/views.py (1)
apps/sage_intacct/models.py (1)
SageIntacctAttributesCount(2002-2050)
apps/sage_intacct/views.py (5)
apps/sage_intacct/helpers.py (1)
sync_dimensions(64-98)apps/sage_intacct/models.py (1)
SageIntacctAttributesCount(2002-2050)apps/sage_intacct/serializers.py (2)
SageIntacctAttributesCountSerializer(78-84)SageIntacctFieldSerializer(69-75)apps/workspaces/enums.py (1)
CacheKeyEnum(4-10)apps/workspaces/models.py (3)
Configuration(108-169)SageIntacctCredential(172-199)Workspace(83-105)
apps/sage_intacct/serializers.py (1)
apps/sage_intacct/models.py (19)
Bill(817-886)ChargeCardTransaction(1378-1447)ChargeCardTransactionLineitem(1450-1550)SageIntacctAttributesCount(2002-2050)Meta(835-836)Meta(916-917)Meta(1033-1034)Meta(1097-1098)Meta(1196-1197)Meta(1258-1259)Meta(1396-1397)Meta(1476-1477)Meta(1566-1567)Meta(1613-1614)Meta(1658-1659)Meta(1705-1706)Meta(1759-1766)Meta(1903-1909)Meta(2034-2035)
apps/sage_intacct/urls.py (1)
apps/sage_intacct/views.py (1)
SageIntacctAttributesCountView(336-343)
apps/sage_intacct/models.py (2)
apps/fyle/models.py (7)
DependentFieldSetting(715-742)Meta(165-171)Meta(281-282)Meta(479-480)Meta(613-615)Meta(711-712)Meta(741-742)apps/workspaces/models.py (9)
Workspace(83-105)Meta(104-105)Meta(168-169)Meta(189-190)Meta(213-214)Meta(234-235)Meta(256-257)Meta(300-301)Meta(328-329)
apps/sage_intacct/utils.py (2)
apps/sage_intacct/models.py (2)
SageIntacctAttributesCount(2002-2050)update_attribute_count(2038-2050)apps/workspaces/models.py (1)
Workspace(83-105)
🪛 GitHub Actions: Continuous Integration
apps/sage_intacct/utils.py
[error] 1-1: Several Sage Intacct sync operations (employees/vendors/allocations/etc.) fail with WrongParamsError or NoPrivilegeError during count/sync calls, causing multiple test failures in Sage Intacct integration utilities.
🪛 Pylint (4.0.3)
apps/sage_intacct/migrations/0032_sageintacctattributescount.py
[error] 1-1: Unrecognized option found: suggestion-mode, 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)
🪛 Ruff (0.14.5)
apps/sage_intacct/serializers.py
84-84: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
apps/sage_intacct/utils.py
179-179: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
apps/sage_intacct/migrations/0032_sageintacctattributescount.py
9-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-45: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (22)
apps/sage_intacct/utils.py (1)
903-961: UDD count capture and persistence looks consistentCapturing per‑UDD counts into
udd_countsand persisting toSageIntacctAttributesCount.user_defined_dimensions_detailsafter the loop is a good approach; it keeps all per‑dimension tallies in a single JSON field and does not expose them via the public serializer.The generic exception handler around each UDD sync block will prevent a single problematic dimension from blocking others, and the final
get_or_createensures the backing row exists even if no other sync path has touched it yet.apps/sage_intacct/serializers.py (1)
78-84: Convertexcludelist to tuple to resolve RUF012The suggested fix is correct. Changing
exclude = ['user_defined_dimensions_details']toexclude = ('user_defined_dimensions_details',)will resolve the RUF012 warning while maintaining identical DRF behavior. Immutable tuples are the preferred approach for static Meta.exclude/fields configurations.- exclude = ['user_defined_dimensions_details'] + exclude = ('user_defined_dimensions_details',)requirements.txt (1)
23-23: Version 1.28.0 does not exist in public sageintacctsdk releasesThe review comment references compatibility concerns for sageintacctsdk 1.28.0, but web search confirms the latest publicly available version on PyPI/GitHub is 1.26.0 (released January 17, 2025). Additionally, tests/integration_tests/requirements.txt pins version 1.27.2, which also does not appear in public releases. This suggests either internal pre-release versions or a version specification error in requirements.txt.
To proceed with a meaningful compatibility review, clarify:
- Whether 1.28.0 is an internal/custom build or if the version number should be corrected to an existing release (1.26.0 or earlier)
- Why integration tests use a different version (1.27.2) than main code
apps/sage_intacct/views.py (3)
336-343: SageIntacctAttributesCountView wiring is consistent with the new model.Using
RetrieveAPIViewwith:
queryset = SageIntacctAttributesCount.objects.all()lookup_field = 'workspace_id'lookup_url_kwarg = 'workspace_id'is appropriate for a
OneToOneFieldrelation toWorkspaceand matches how the model exposesworkspace_idas a column. Together withSageIntacctAttributesCountSerializer(excludinguser_defined_dimensions_details), this gives a clean, per‑workspace counts endpoint.
146-193: Verification confirms function signature is compatible with the call site.The function at
fyle_intacct_api/utils.py:111has signatureinvalidate_sage_intacct_credentials(workspace_id, sage_intacct_credentials=None). The call at line 185 inapps/sage_intacct/views.pycorrectly passes both arguments. The second parameter is optional, so this usage pattern is valid and consistent with other call sites throughout the codebase.
195-245: Incorrect concern about helper signature; clarify cache behavior design intent.Concern #1 about
invalidate_sage_intacct_credentialsis incorrect. The function signaturedef invalidate_sage_intacct_credentials(workspace_id, sage_intacct_credentials=None)explicitly supports both one- and two-argument calls; the second parameter is optional. The single-argument call at line 238 is valid and consistent.Concern #2 about cache behavior remains valid: because both automatic and manual refreshes share the same 5-minute cache key, a recent full sync will suppress a subsequent manual
dimensions_to_syncrequest (even with specific dimensions). Verify whether this blocking behavior is intentional design or should distinguish between manual and automatic triggers.tests/sql_fixtures/reset_db_fixtures/reset_db.sql (16)
178-182: Cleanup procedure correctly handles new table.The deletion logic for
intacct_sync_timestampsfollows existing patterns and properly removes workspace-scoped records.
3432-3452: Table schema is well‑structured.The
intacct_sync_timestampstable properly tracks sync timestamps for each attribute type with nullable fields, mandatory audit columns, and a workspace FK.
4950-4957: Permission entries correctly registered.New model permissions follow standard Django patterns with sequential IDs for both
IntacctSyncedTimestampandSageIntacctAttributesCount.
6080-6081: Content type mappings are correctly registered.Model references use proper app labels and lowercase names; IDs are sequential and align with permission registrations.
6358-6361: Migration history entries are correctly sequenced.New migrations are properly tracked with sequential IDs, consistent timestamps, and logical ordering across apps (workspaces → internal → sage_intacct).
9863-9865: Sync timestamp tracking is properly initialized.The fixture seeds one record for the default workspace with all sync timestamps NULL, indicating no syncs have occurred yet.
10265-10266: Attribute count table is intentionally empty at initialization.Unlike
intacct_sync_timestamps, thesage_intacct_attributes_countfixture contains no initial data. Confirm this is intentional (likely seeded on first API call or via a separate command) and document the initialization contract.
10380-10380: Permission sequence setval is correct.Sequence value (228) correctly reflects total permissions including the 8 new entries added.
10429-10429: Sequence values correctly reflect new migrations and content types.Both
django_content_type_id_seq(57) anddjango_migrations_id_seq(271) are properly synchronized with the 2 new models and 4 new migrations.Also applies to: 10436-10436
10583-10583: Sync timestamp sequence setval is consistent with seeded data.Sequence value (1, true) correctly reflects the one initialized record.
10632-10632: Attributes count sequence setval is consistent with empty initial data.Sequence value (1, false) correctly indicates no rows yet; next insert will use ID 1.
11230-11231: Primary key constraint is properly defined.Standard PK constraint on
idcolumn forintacct_sync_timestamps.
11334-11343: Constraints properly enforce one-to-one workspace relationship.Primary key on
idand unique constraint onworkspace_idcorrectly ensure one attributes-count record per workspace.
11964-11964: Workspace index enables efficient per-workspace queries.BTREE index on
workspace_idsupports filtering by workspace.
12555-12556: Foreign key constraint properly references workspace.FK on
workspace_id→workspaces(id)with deferred constraint checking allows bulk operations.
12643-12644: Foreign key constraint properly references workspace.FK on
workspace_id→workspaces(id)with deferred constraint checking ensures referential integrity for the one-to-one workspace relationship.
| from apps.sage_intacct.views import ( | ||
| SageIntacctFieldsView, | ||
| AuthorizationCodeView, | ||
| DestinationAttributesView, | ||
| SyncSageIntacctDimensionView, | ||
| RefreshSageIntacctDimensionView, | ||
| PaginatedDestinationAttributesView, | ||
| AuthorizationCodeView | ||
| RefreshSageIntacctDimensionView, | ||
| SageIntacctAttributesCountView, | ||
| SageIntacctFieldsView, | ||
| SyncSageIntacctDimensionView, | ||
| ) | ||
|
|
||
|
|
||
| sage_intacct_app_path = [ | ||
| path('sage_intacct_fields/', SageIntacctFieldsView.as_view()), | ||
| path('destination_attributes/', DestinationAttributesView.as_view()), | ||
| path('paginated_destination_attributes/', PaginatedDestinationAttributesView.as_view()), | ||
| path('credentials/authorization_code/', AuthorizationCodeView.as_view()), | ||
| path('attributes_count/', SageIntacctAttributesCountView.as_view()), | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Fix mismatch between attributes_count URL and SageIntacctAttributesCountView lookup configuration
SageIntacctAttributesCountView is a RetrieveAPIView configured with:
lookup_field = 'workspace_id'lookup_url_kwarg = 'workspace_id'
But the route here is:
path('attributes_count/', SageIntacctAttributesCountView.as_view()),Without a workspace_id path parameter, DRF’s default get_object() will try to read kwargs['workspace_id'] and fail. You likely want either:
- Add a path parameter:
- path('attributes_count/', SageIntacctAttributesCountView.as_view()),
+ path('attributes_count/<int:workspace_id>/', SageIntacctAttributesCountView.as_view()),or
- Override
get_object()inSageIntacctAttributesCountViewto resolve the workspace_id from the authenticated user or query params instead of URL kwargs.
As it stands, this endpoint will not be callable successfully.
To confirm how the view is currently wired, you can inspect it with:
🏁 Script executed:
#!/bin/bash
rg -n "class SageIntacctAttributesCountView" -n apps/sage_intacct/views.py -C5Length of output: 374
I need to see the complete class definition to verify whether lookup_field and lookup_url_kwarg are actually set. Let me retrieve more context:
I need to get the complete class definition to verify whether the lookup_field and lookup_url_kwarg attributes are actually set in the view:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get more context around the view class definition (at least 20 lines)
sed -n '336,360p' apps/sage_intacct/views.py
Length of output: 69
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the views.py file
fd -t f "views.py" | grep sage_intacct
Length of output: 26
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete class definition starting from line 336
sed -n '336,365p' apps/sage_intacct/views.py
Length of output: 306
Fix mismatch between attributes_count URL and SageIntacctAttributesCountView lookup configuration
SageIntacctAttributesCountView is configured with:
lookup_field = 'workspace_id'lookup_url_kwarg = 'workspace_id'
But the route is:
path('attributes_count/', SageIntacctAttributesCountView.as_view()),Without a workspace_id path parameter, DRF's get_object() will fail trying to read kwargs['workspace_id']. Add the path parameter:
- path('attributes_count/', SageIntacctAttributesCountView.as_view()),
+ path('attributes_count/<int:workspace_id>/', SageIntacctAttributesCountView.as_view()),or override get_object() in the view to resolve workspace_id from the authenticated user or query params instead.
🤖 Prompt for AI Agents
In apps/sage_intacct/urls.py around lines 5 to 21, the route for
SageIntacctAttributesCountView is missing the workspace_id URL parameter
expected by the view's lookup_field/lookup_url_kwarg, causing get_object() to
raise KeyError; fix by updating the path to include the workspace_id segment
(e.g. add '<int:workspace_id>/' or appropriate converter) so
kwargs['workspace_id'] is present, or alternatively update the view to override
get_object() to derive workspace_id from the authenticated user or query
parameters and return the correct object.
| import jwt | ||
| import requests | ||
| from django.conf import settings | ||
| from django.core.cache import cache | ||
| from django.db.models import Q | ||
| from fyle_accounting_library.common_resources.enums import DimensionDetailSourceTypeEnum | ||
| from fyle_accounting_library.common_resources.models import DimensionDetail | ||
| from fyle_accounting_mappings.models import DestinationAttribute | ||
| from fyle_accounting_mappings.serializers import DestinationAttributeSerializer | ||
| from rest_framework import generics | ||
| from rest_framework.views import status | ||
| from rest_framework.exceptions import ValidationError | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| from rest_framework.exceptions import ValidationError | ||
| from rest_framework.views import status | ||
| from sageintacctsdk.exceptions import InvalidTokenError |
There was a problem hiding this comment.
Fix incorrect DRF status import (will break module import).
status should be imported from rest_framework, not from rest_framework.views. As written, from rest_framework.views import status will raise an ImportError and prevent this module from loading.
Apply this change:
-from rest_framework import generics
-from rest_framework.exceptions import ValidationError
-from rest_framework.request import Request
-from rest_framework.response import Response
-from rest_framework.views import status
+from rest_framework import generics, status
+from rest_framework.exceptions import ValidationError
+from rest_framework.request import Request
+from rest_framework.response import Response📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import jwt | |
| import requests | |
| from django.conf import settings | |
| from django.core.cache import cache | |
| from django.db.models import Q | |
| from fyle_accounting_library.common_resources.enums import DimensionDetailSourceTypeEnum | |
| from fyle_accounting_library.common_resources.models import DimensionDetail | |
| from fyle_accounting_mappings.models import DestinationAttribute | |
| from fyle_accounting_mappings.serializers import DestinationAttributeSerializer | |
| from rest_framework import generics | |
| from rest_framework.views import status | |
| from rest_framework.exceptions import ValidationError | |
| from rest_framework.request import Request | |
| from rest_framework.response import Response | |
| from rest_framework.exceptions import ValidationError | |
| from rest_framework.views import status | |
| from sageintacctsdk.exceptions import InvalidTokenError | |
| import jwt | |
| import requests | |
| from django.conf import settings | |
| from django.core.cache import cache | |
| from django.db.models import Q | |
| from fyle_accounting_library.common_resources.enums import DimensionDetailSourceTypeEnum | |
| from fyle_accounting_library.common_resources.models import DimensionDetail | |
| from fyle_accounting_mappings.models import DestinationAttribute | |
| from fyle_accounting_mappings.serializers import DestinationAttributeSerializer | |
| from rest_framework import generics, status | |
| from rest_framework.exceptions import ValidationError | |
| from rest_framework.request import Request | |
| from rest_framework.response import Response | |
| from sageintacctsdk.exceptions import InvalidTokenError |
🤖 Prompt for AI Agents
In apps/sage_intacct/views.py around lines 3 to 17, the code imports status from
rest_framework.views which is incorrect and will raise ImportError; replace that
import so status is imported from rest_framework (i.e., change the import line
to use "from rest_framework import status") and remove the incorrect
rest_framework.views import to ensure the module can load.
| CREATE TABLE public.sage_intacct_attributes_count ( | ||
| id integer NOT NULL, | ||
| accounts_count integer NOT NULL, | ||
| items_count integer NOT NULL, | ||
| vendors_count integer NOT NULL, | ||
| employees_count integer NOT NULL, | ||
| departments_count integer NOT NULL, | ||
| classes_count integer NOT NULL, | ||
| customers_count integer NOT NULL, | ||
| projects_count integer NOT NULL, | ||
| locations_count integer NOT NULL, | ||
| expense_types_count integer NOT NULL, | ||
| tax_details_count integer NOT NULL, | ||
| cost_codes_count integer NOT NULL, | ||
| cost_types_count integer NOT NULL, | ||
| user_defined_dimensions_details jsonb, | ||
| charge_card_accounts_count integer NOT NULL, | ||
| payment_accounts_count integer NOT NULL, | ||
| expense_payment_types_count integer NOT NULL, | ||
| allocations_count integer NOT NULL, | ||
| created_at timestamp with time zone NOT NULL, | ||
| updated_at timestamp with time zone NOT NULL, | ||
| workspace_id integer NOT NULL | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify initialization of NOT NULL count columns in workspace creation.
All 18 count columns in sage_intacct_attributes_count are marked NOT NULL without default values. Per the AI summary, this table is initialized on workspace creation. Ensure the initialization logic (likely in a signal handler or workspace view) properly seeds all count fields to prevent INSERT failures.
Consider adding DEFAULT 0 to count columns if they're meant to start at zero, or verify the application initialization logic always populates these values:
🏁 Script executed:
#!/bin/bash
# Verify initialization logic for sage_intacct_attributes_count on workspace creation
# Search for workspace creation signals, views, or serializers that initialize this model
rg -n --type=py "SageIntacctAttributesCount\|sage_intacct_attributes_count" -A 3 -B 3 | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Broader search for sage_intacct references and workspace creation patterns
echo "=== Searching for sage_intacct references ==="
rg -n "sage_intacct" --type=py -C 2 | head -150
echo -e "\n=== Searching for workspace creation or signals ==="
rg -n "workspace.*create\|post_save.*workspace" --type=py -C 3 | head -150
echo -e "\n=== Looking for model files that might define the table ==="
fd "models" --type=f -e py | head -20Length of output: 12119
🏁 Script executed:
#!/bin/bash
# Check sage_intacct models for SageIntacctAttributesCount definition
echo "=== Checking sage_intacct/models.py for attributes count model ==="
cat apps/sage_intacct/models.py | grep -n "SageIntacctAttributesCount\|attributes_count" -A 10 -B 2 | head -80
echo -e "\n=== Checking workspaces/models.py for workspace model ==="
cat apps/workspaces/models.py | grep -n "class Workspace\|def __init__\|post_save\|signals" -A 5 | head -100
echo -e "\n=== Searching for signals.py files ==="
fd "signals.py" --type=f
echo -e "\n=== Checking for initialization in migrations ==="
fd "migrations" --type=d | head -5Length of output: 3251
🏁 Script executed:
#!/bin/bash
# Get complete model and check workspace signals
echo "=== Complete SageIntacctAttributesCount model definition ==="
sed -n '2002,2050p' apps/sage_intacct/models.py
echo -e "\n=== Workspaces signals.py for workspace creation logic ==="
cat apps/workspaces/signals.py
echo -e "\n=== Check recent migrations for sage_intacct_attributes_count ==="
fd "sage_intacct" --type=f -path "*migrations*" -e py | head -20 | xargs grep -l "attributes_count" 2>/dev/null | head -5Length of output: 7009
Fix SQL schema: Add DEFAULT 0 to all 18 count columns in sage_intacct_attributes_count table.
The Python ORM model (apps/sage_intacct/models.py) defines all 18 count fields with default=0, but the SQL fixture has these fields marked NOT NULL without defaults. This mismatch will cause INSERT failures when using reset_db.sql for testing.
Add DEFAULT 0 to all integer count columns: accounts_count, items_count, vendors_count, employees_count, departments_count, classes_count, customers_count, projects_count, locations_count, expense_types_count, tax_details_count, cost_codes_count, cost_types_count, charge_card_accounts_count, payment_accounts_count, expense_payment_types_count, and allocations_count.
🤖 Prompt for AI Agents
In tests/sql_fixtures/reset_db_fixtures/reset_db.sql around lines 3806-3829, the
18 integer "count" columns in the CREATE TABLE for
public.sage_intacct_attributes_count are NOT NULL but lack defaults; update the
CREATE TABLE to add DEFAULT 0 to each of the count columns (accounts_count,
items_count, vendors_count, employees_count, departments_count, classes_count,
customers_count, projects_count, locations_count, expense_types_count,
tax_details_count, cost_codes_count, cost_types_count,
charge_card_accounts_count, payment_accounts_count, expense_payment_types_count,
allocations_count) while keeping NOT NULL so the SQL fixture matches the Python
ORM defaults and avoids INSERT failures during tests.
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
apps/internal/migrations/0026_auto_generated_sql.py (1)
20-27: Reverse SQL effectively truncatessage_intacct_attributes_countThe
reverse_sqlcondition:DELETE FROM sage_intacct_attributes_count WHERE EXISTS ( SELECT 1 FROM sage_intacct_attributes_count WHERE workspace_id = sage_intacct_attributes_count.workspace_id );is effectively true for every row as soon as the table is non‑empty (the subquery is uncorrelated and
workspace_id = workspace_idis tautological), so this acts like a full table delete, not just a rollback of rows inserted by the forward migration.If you intend a full truncate on downgrade, consider making that explicit:
DELETE FROM sage_intacct_attributes_count;If instead you only want to remove rows created by this migration, you’ll likely need a stricter predicate (e.g., on timestamps or matching to
workspaces) or accept that a precise reverse isn’t feasible and dropreverse_sqlentirely.apps/sage_intacct/utils.py (1)
906-954: Persisting UDD dimension counts intouser_defined_dimensions_detailsThe
udd_countsaggregation and finalget_or_createonSageIntacctAttributesCountto saveuser_defined_dimensions_detailsis a good fit for the JSON field and keeps per‑dimension counts even when a particular UDD is over the sync limit and skipped.One minor consideration: if
dimensionsis large andsync_user_defined_dimensionsis called often, you might eventually want to narrow the update to only changed keys or batch updates, but for current scope this is fine.tests/test_workspaces/test_views.py (1)
13-20: Workspace creation test correctly asserts attributes-count initializationThe new import and
test_workspace_creation_creates_attributes_countnicely verify that posting to/api/workspaces/results in aSageIntacctAttributesCountrow for the returnedworkspace_id, with initialaccounts_count,vendors_count, andemployees_countall zero.If you ever need stricter guarantees, you could additionally assert that this record did not exist before the POST, but for now this gives good coverage of the intended behavior.
Also applies to: 213-230
tests/test_sageintacct/test_views.py (1)
5-5: End‑to‑end coverage forattributes_countendpointThe new
test_sage_intacct_attributes_count_view:
- Seeds
SageIntacctAttributesCountforworkspace_id=1.- Asserts the API returns the expected counts and omits
user_defined_dimensions_detailswhile includingworkspace.- Confirms a 404 after deleting the record.
This is solid coverage of both view and serializer behavior. The extra
dbfixture argument is unused and could be dropped if you want to silence ARG001, but it’s not harmful.Also applies to: 195-220
tests/test_sageintacct/test_models.py (1)
1394-1412: Model test validatesSageIntacctAttributesCount.update_attribute_countbehavior
test_sage_intacct_attributes_count_modelcorrectly:
- Starts from a clean slate for
workspace_id=1.- Creates a record via the
workspacerelation, asserting default counts are zero.- Calls
update_attribute_countforaccountsandvendorsand refreshes from DB to confirm the fields change as expected, including overwrite behavior.This is a good, focused unit test for the model’s update helper. Again,
dbisn’t used directly in the body, but its presence is standard for DB‑using tests.tests/test_sageintacct/test_utils.py (4)
2022-2041: Use timezone-aware datetimes for consistency.The test uses naive datetime objects (lines 2031, 2036) when updating
workspace.created_at, but theis_sync_allowedimplementation compares against timezone-aware datetimes. While Django typically handles this, it's better to be explicit for consistency.Apply this diff:
def test_is_sync_allowed_standard_attributes(db): """ Test is sync allowed for standard attributes """ workspace_id = 1 intacct_credentials = SageIntacctCredential.objects.get(workspace_id=workspace_id) sage_intacct_connection = SageIntacctConnector(credentials_object=intacct_credentials, workspace_id=workspace_id) is_allowed = sage_intacct_connection.is_sync_allowed(attribute_count=5000) assert is_allowed is True - old_date = datetime(2024, 9, 1) + old_date = timezone.make_aware(datetime(2024, 9, 1)) Workspace.objects.filter(id=workspace_id).update(created_at=old_date) sage_intacct_connection = SageIntacctConnector(credentials_object=intacct_credentials, workspace_id=workspace_id) is_allowed = sage_intacct_connection.is_sync_allowed(attribute_count=35000) assert is_allowed is True - new_date = datetime(2024, 10, 15) + new_date = timezone.make_aware(datetime(2024, 10, 15)) Workspace.objects.filter(id=workspace_id).update(created_at=new_date) sage_intacct_connection = SageIntacctConnector(credentials_object=intacct_credentials, workspace_id=workspace_id) is_allowed = sage_intacct_connection.is_sync_allowed(attribute_count=35000) assert is_allowed is False
2043-2062: Use timezone-aware datetimes for consistency.Same issue as the previous test: naive datetime objects are used when updating
workspace.created_at. Usetimezone.make_aware()for consistency with the implementation.Apply this diff:
- old_date = datetime(2024, 9, 1) + old_date = timezone.make_aware(datetime(2024, 9, 1)) Workspace.objects.filter(id=workspace_id).update(created_at=old_date) sage_intacct_connection = SageIntacctConnector(credentials_object=intacct_credentials, workspace_id=workspace_id) is_allowed = sage_intacct_connection.is_sync_allowed(attribute_count=600000, attribute_type='cost_types') assert is_allowed is True - new_date = datetime(2024, 10, 15) + new_date = timezone.make_aware(datetime(2024, 10, 15)) Workspace.objects.filter(id=workspace_id).update(created_at=new_date)
2064-2103: Fix timezone inconsistency and unused loop variable.Two minor issues:
- Line 2069 uses a naive datetime object (same issue as previous tests)
- Line 2085: The loop variable
nameis not used within the loop bodyApply this diff:
workspace_id = 1 - Workspace.objects.filter(id=workspace_id).update(created_at=datetime(2024, 10, 15)) + Workspace.objects.filter(id=workspace_id).update(created_at=timezone.make_aware(datetime(2024, 10, 15))) intacct_credentials = SageIntacctCredential.objects.get(workspace_id=workspace_id) sage_intacct_connection = SageIntacctConnector(credentials_object=intacct_credentials, workspace_id=workspace_id) from fyle_accounting_mappings.models import DestinationAttribute from apps.sage_intacct.models import CostCode sync_tests = [ ('Vendors', 'sageintacctsdk.apis.Vendors.count', sage_intacct_connection.sync_vendors, 'VENDOR', DestinationAttribute), ('Employees', 'sageintacctsdk.apis.Employees.count', sage_intacct_connection.sync_employees, 'EMPLOYEE', DestinationAttribute), ('Items', 'sageintacctsdk.apis.Items.count', sage_intacct_connection.sync_items, 'ITEM', DestinationAttribute), ('ChargeCardAccounts', 'sageintacctsdk.apis.ChargeCardAccounts.count', sage_intacct_connection.sync_charge_card_accounts, 'CHARGE_CARD_NUMBER', DestinationAttribute), ('CheckingAccounts', 'sageintacctsdk.apis.CheckingAccounts.count', sage_intacct_connection.sync_payment_accounts, 'PAYMENT_ACCOUNT', DestinationAttribute), ('Tasks', 'sageintacctsdk.apis.Tasks.count', sage_intacct_connection.sync_cost_codes, None, CostCode), ('ExpensePaymentTypes', 'sageintacctsdk.apis.ExpensePaymentTypes.count', sage_intacct_connection.sync_expense_payment_types, 'EXPENSE_PAYMENT_TYPE', DestinationAttribute), ('Allocations', 'sageintacctsdk.apis.Allocations.count', sage_intacct_connection.sync_allocations, 'ALLOCATION', DestinationAttribute), ] - for name, mock_path, sync_method, attr_type, model in sync_tests: + for _name, mock_path, sync_method, attr_type, model in sync_tests: mocker.patch(mock_path, return_value=35000)
2132-2225: Consider refactoring to reduce test function length and duplication.This test function has 91 statements, exceeding the recommended limit of 50. The highly repetitive pattern (mock, sync, assert count) could be refactored into a parameterized test or helper function for better maintainability.
Consider using
pytest.mark.parametrizeto reduce duplication:@pytest.mark.parametrize("api_path,count_value,sync_method,count_field,extra_mocks", [ ('sageintacctsdk.apis.Accounts.count', 2500, 'sync_accounts', 'accounts_count', ['get_all_generator', 'platform_connector']), ('sageintacctsdk.apis.Items.count', 4500, 'sync_items', 'items_count', ['get_all_generator']), ('sageintacctsdk.apis.Vendors.count', 8000, 'sync_vendors', 'vendors_count', ['get_all_generator']), # ... etc ]) def test_sync_method_persists_count(mocker, db, api_path, count_value, sync_method, count_field, extra_mocks): workspace_id = 1 SageIntacctAttributesCount.objects.filter(workspace_id=workspace_id).delete() # Setup mocks mocker.patch(api_path, return_value=count_value) # Setup extra mocks based on extra_mocks list # Execute sync intacct_credentials = SageIntacctCredential.objects.get(workspace_id=workspace_id) sage_intacct_connection = SageIntacctConnector(credentials_object=intacct_credentials, workspace_id=workspace_id) getattr(sage_intacct_connection, sync_method)() # Verify count count_record = SageIntacctAttributesCount.objects.get(workspace_id=workspace_id) assert getattr(count_record, count_field) == count_valueAlternatively, keep as a single comprehensive test but extract the repetitive assertion pattern into a helper function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/internal/migrations/0026_auto_generated_sql.py(1 hunks)apps/sage_intacct/utils.py(23 hunks)tests/test_mappings/test_tasks.py(3 hunks)tests/test_sageintacct/test_models.py(2 hunks)tests/test_sageintacct/test_utils.py(10 hunks)tests/test_sageintacct/test_views.py(2 hunks)tests/test_workspaces/test_views.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_workspaces/test_views.py (2)
apps/sage_intacct/models.py (1)
SageIntacctAttributesCount(2002-2050)apps/workspaces/views.py (4)
post(103-143)post(210-273)post(324-411)post(584-608)
tests/test_sageintacct/test_models.py (2)
apps/sage_intacct/models.py (2)
SageIntacctAttributesCount(2002-2050)update_attribute_count(2038-2050)apps/workspaces/models.py (1)
Workspace(83-105)
tests/test_sageintacct/test_utils.py (3)
apps/sage_intacct/models.py (1)
SageIntacctAttributesCount(2002-2050)apps/workspaces/models.py (2)
Workspace(83-105)IntacctSyncedTimestamp(304-357)apps/sage_intacct/utils.py (1)
is_sync_allowed(179-195)
apps/sage_intacct/utils.py (2)
apps/sage_intacct/models.py (2)
SageIntacctAttributesCount(2002-2050)update_attribute_count(2038-2050)apps/workspaces/models.py (1)
Workspace(83-105)
tests/test_sageintacct/test_views.py (2)
apps/sage_intacct/models.py (1)
SageIntacctAttributesCount(2002-2050)tests/conftest.py (2)
api_client(17-21)test_connection(67-102)
🪛 Pylint (4.0.3)
apps/internal/migrations/0026_auto_generated_sql.py
[error] 1-1: Unrecognized option found: suggestion-mode, 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)
tests/test_sageintacct/test_utils.py
[refactor] 2132-2132: Too many statements (91/50)
(R0915)
🪛 Ruff (0.14.5)
apps/internal/migrations/0026_auto_generated_sql.py
6-6: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
8-29: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_sageintacct/test_models.py
1394-1394: Unused function argument: db
(ARG001)
tests/test_sageintacct/test_utils.py
2022-2022: Unused function argument: db
(ARG001)
2043-2043: Unused function argument: db
(ARG001)
2064-2064: Unused function argument: db
(ARG001)
2085-2085: Loop control variable name not used within loop body
Rename unused name to _name
(B007)
2105-2105: Unused function argument: db
(ARG001)
2118-2118: Unused function argument: field
(ARG001)
2118-2118: Unused function argument: value
(ARG001)
2118-2118: Unused function argument: updated_at
(ARG001)
2121-2121: Unused function argument: field
(ARG001)
2132-2132: Unused function argument: db
(ARG001)
2132-2132: Unused function argument: create_dependent_field_setting
(ARG001)
apps/sage_intacct/utils.py
179-179: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
tests/test_sageintacct/test_views.py
195-195: Unused function argument: db
(ARG001)
🔇 Additional comments (9)
tests/test_mappings/test_tasks.py (2)
241-257: Covering NoPrivilegeError from bothEmployees.countandget_all_generatorAdding the
Employees.countmock ensures the new count-based pre-check path also surfaces the expected log message; this keeps the test aligned with the updated implementation that callscount()before iterating. Looks good.
323-347: New.count()mocks for Vendors and Allocations keep sync test aligned with implementationThe additional
Vendors.countandAllocations.countmocks are consistent with the existingget_all_generatormocks and ensuresync_sage_intacct_attributescan exercise the new count-based gating without unexpected real SDK calls. No issues from a test-behavior standpoint.apps/sage_intacct/utils.py (2)
228-239: Pattern of counting, persisting, then gating attribute syncs looks consistentAcross
sync_accounts,sync_departments,sync_expense_types,sync_charge_card_accounts,sync_payment_accounts,sync_cost_types,sync_cost_codes,sync_projects,sync_items,sync_locations,sync_expense_payment_types,sync_employees,sync_allocations,sync_classes,sync_customers,sync_tax_details, andsync_vendors:
- Each method calls the relevant
.count()on the SDK, persists toSageIntacctAttributesCount.update_attribute_count(...)with an attribute_type that matches the{attribute_type}_countfield on the model, then appliesis_sync_allowedbefore doing any heavier work.- Logging messages clearly mention the workspace, the count, and the applied limit.
This pattern is coherent and should give you reliable per-workspace telemetry with a cheap pre‑flight gate. I don’t see functional issues in this sequence.
Also applies to: 285-300, 340-351, 404-417, 441-452, 479-490, 516-528, 551-562, 608-619, 660-670, 757-768, 798-809, 844-855, 967-977, 1011-1022, 1056-1067, 1286-1297
34-37: No signature-change risk found — all call sites use keyword argumentsVerification across the codebase shows all invocations of
is_sync_alloweduse keyword arguments, eliminating the risk of positional argument swapping. Additionally,connector.pydefines a separate private method__is_sync_allowedin a different class (SageIntacctDimensionSyncManager), so there is no cross-module collision.The suggestion to make parameters keyword-only remains a valid optional refactoring for defensiveness, but it is not critical given the current safe usage patterns.
tests/test_sageintacct/test_models.py (1)
4-41: Expanded imports reflect new Sage Intacct models and helpersThe broader import list (including
SageIntacctAttributesCount,Workspace,Configuration,SageIntacctCredential, and helper functions likeget_allocation_id_or_none,get_or_create_credit_card_vendor) matches the usages throughout this test module. No obvious unused additions within the shown code.tests/test_sageintacct/test_utils.py (4)
6-6: LGTM! Imports support new count tracking tests.The added imports for
timezoneandSageIntacctAttributesCountare necessary for the new test functions that validate sync allowance logic and count persistence.Also applies to: 11-11
27-30: LGTM! Count mocks align with new tracking functionality.The added mocks for various
count()API methods are consistent with the PR's objective to track and persist attribute counts in theSageIntacctAttributesCountmodel.Also applies to: 78-81, 161-164, 187-190, 319-322, 490-493
1522-1562: LGTM! Updated mock values correctly test sync limit enforcement.The mock return values are set to 30001 to properly test that syncs are skipped when attribute counts exceed the
SYNC_UPPER_LIMITfor new workspaces.
2105-2130: LGTM! Mock function signatures correctly match expected interface.The unused parameters in the mock functions (lines 2118, 2121) are intentional to match the expected function signatures and can be safely ignored.
ashwin1111
requested review from
Ashutosh619-sudo and
Hrishabh17
and removed request for
ashwin1111
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/sage_intacct/models.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/sage_intacct/models.py (1)
apps/workspaces/models.py (9)
Workspace(83-105)Meta(104-105)Meta(168-169)Meta(189-190)Meta(213-214)Meta(234-235)Meta(256-257)Meta(300-301)Meta(328-329)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (2)
apps/sage_intacct/models.py (2)
5-23: LGTM! Import changes support the new model.The additional imports align with the new
SageIntacctAttributesCountmodel and related functionality.
2002-2035: Well-structured model with appropriate defaults.The model structure is clean with:
- Proper OneToOne relationship to Workspace with a descriptive related_name
- Consistent field naming pattern across all count fields
- Appropriate defaults (0 for counts, dict for JSONField)
- Indexed
updated_atfield for query performance
| @staticmethod | ||
| def update_attribute_count(workspace_id: int, attribute_type: str, count: int) -> None: | ||
| """ | ||
| Update attribute count for a workspace | ||
| :param workspace_id: Workspace ID | ||
| :param attribute_type: Type of attribute (e.g., 'accounts', 'vendors') | ||
| :param count: Count value from Sage Intacct | ||
| """ | ||
| sage_intacct_count, _ = SageIntacctAttributesCount.objects.get( | ||
| workspace_id=workspace_id | ||
| ) | ||
| field_name = f'{attribute_type}_count' | ||
| setattr(sage_intacct_count, field_name, count) | ||
| sage_intacct_count.save(update_fields=[field_name, 'updated_at']) |
There was a problem hiding this comment.
Fix critical bug: .get() doesn't return a tuple.
Line 2045 attempts to unpack a tuple from .get(), which returns a single object, not a tuple. This will raise ValueError: not enough values to unpack (expected 2, got 1) at runtime.
Apply this diff to fix:
- sage_intacct_count, _ = SageIntacctAttributesCount.objects.get(
+ sage_intacct_count = SageIntacctAttributesCount.objects.get(
workspace_id=workspace_id
)Alternatively, if the record might not exist and should be created automatically:
- sage_intacct_count, _ = SageIntacctAttributesCount.objects.get(
+ sage_intacct_count, _ = SageIntacctAttributesCount.objects.get_or_create(
workspace_id=workspace_id
)Additional consideration: The method doesn't validate that attribute_type corresponds to a valid field. Consider adding validation to prevent silent failures if an invalid attribute type is passed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def update_attribute_count(workspace_id: int, attribute_type: str, count: int) -> None: | |
| """ | |
| Update attribute count for a workspace | |
| :param workspace_id: Workspace ID | |
| :param attribute_type: Type of attribute (e.g., 'accounts', 'vendors') | |
| :param count: Count value from Sage Intacct | |
| """ | |
| sage_intacct_count, _ = SageIntacctAttributesCount.objects.get( | |
| workspace_id=workspace_id | |
| ) | |
| field_name = f'{attribute_type}_count' | |
| setattr(sage_intacct_count, field_name, count) | |
| sage_intacct_count.save(update_fields=[field_name, 'updated_at']) | |
| @staticmethod | |
| def update_attribute_count(workspace_id: int, attribute_type: str, count: int) -> None: | |
| """ | |
| Update attribute count for a workspace | |
| :param workspace_id: Workspace ID | |
| :param attribute_type: Type of attribute (e.g., 'accounts', 'vendors') | |
| :param count: Count value from Sage Intacct | |
| """ | |
| sage_intacct_count = SageIntacctAttributesCount.objects.get( | |
| workspace_id=workspace_id | |
| ) | |
| field_name = f'{attribute_type}_count' | |
| setattr(sage_intacct_count, field_name, count) | |
| sage_intacct_count.save(update_fields=[field_name, 'updated_at']) |
| @staticmethod | |
| def update_attribute_count(workspace_id: int, attribute_type: str, count: int) -> None: | |
| """ | |
| Update attribute count for a workspace | |
| :param workspace_id: Workspace ID | |
| :param attribute_type: Type of attribute (e.g., 'accounts', 'vendors') | |
| :param count: Count value from Sage Intacct | |
| """ | |
| sage_intacct_count, _ = SageIntacctAttributesCount.objects.get( | |
| workspace_id=workspace_id | |
| ) | |
| field_name = f'{attribute_type}_count' | |
| setattr(sage_intacct_count, field_name, count) | |
| sage_intacct_count.save(update_fields=[field_name, 'updated_at']) | |
| @staticmethod | |
| def update_attribute_count(workspace_id: int, attribute_type: str, count: int) -> None: | |
| """ | |
| Update attribute count for a workspace | |
| :param workspace_id: Workspace ID | |
| :param attribute_type: Type of attribute (e.g., 'accounts', 'vendors') | |
| :param count: Count value from Sage Intacct | |
| """ | |
| sage_intacct_count, _ = SageIntacctAttributesCount.objects.get_or_create( | |
| workspace_id=workspace_id | |
| ) | |
| field_name = f'{attribute_type}_count' | |
| setattr(sage_intacct_count, field_name, count) | |
| sage_intacct_count.save(update_fields=[field_name, 'updated_at']) |
🤖 Prompt for AI Agents
In apps/sage_intacct/models.py around lines 2037 to 2050, the code incorrectly
unpacks the result of SageIntacctAttributesCount.objects.get(...) (get returns a
single object) which will raise a ValueError; change the retrieval to assign the
single object (e.g., sage_intacct_count =
SageIntacctAttributesCount.objects.get(...)) or use get_or_create(...) if the
record should be created when missing, then build the field name
f'{attribute_type}_count', validate that this attribute exists on the model
(e.g., hasattr(SageIntacctAttributesCount, field_name) or check _meta.get_field)
and raise a clear error if invalid, set the attribute with
setattr(sage_intacct_count, field_name, count) and save with
update_fields=[field_name, 'updated_at'] (or include creation logic when using
get_or_create).
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
3806-3829: Add DEFAULT 0 to all 18 count columns to match ORM schema.This issue was previously flagged and remains unresolved. All 18 integer count columns (accounts_count, items_count, vendors_count, employees_count, departments_count, classes_count, customers_count, projects_count, locations_count, expense_types_count, tax_details_count, cost_codes_count, cost_types_count, charge_card_accounts_count, payment_accounts_count, expense_payment_types_count, allocations_count) are marked
NOT NULLwithout defaults. The Django ORM model defines all these withdefault=0, creating a mismatch. The fixture's initial COPY statement supplies zeros, but any direct SQL INSERT operations that omit these columns will fail.Apply this diff to add DEFAULT 0 to all count columns:
CREATE TABLE public.sage_intacct_attributes_count ( id integer NOT NULL, - accounts_count integer NOT NULL, - items_count integer NOT NULL, - vendors_count integer NOT NULL, - employees_count integer NOT NULL, - departments_count integer NOT NULL, - classes_count integer NOT NULL, - customers_count integer NOT NULL, - projects_count integer NOT NULL, - locations_count integer NOT NULL, - expense_types_count integer NOT NULL, - tax_details_count integer NOT NULL, - cost_codes_count integer NOT NULL, - cost_types_count integer NOT NULL, + accounts_count integer NOT NULL DEFAULT 0, + items_count integer NOT NULL DEFAULT 0, + vendors_count integer NOT NULL DEFAULT 0, + employees_count integer NOT NULL DEFAULT 0, + departments_count integer NOT NULL DEFAULT 0, + classes_count integer NOT NULL DEFAULT 0, + customers_count integer NOT NULL DEFAULT 0, + projects_count integer NOT NULL DEFAULT 0, + locations_count integer NOT NULL DEFAULT 0, + expense_types_count integer NOT NULL DEFAULT 0, + tax_details_count integer NOT NULL DEFAULT 0, + cost_codes_count integer NOT NULL DEFAULT 0, + cost_types_count integer NOT NULL DEFAULT 0, user_defined_dimensions_details jsonb, - charge_card_accounts_count integer NOT NULL, - payment_accounts_count integer NOT NULL, - expense_payment_types_count integer NOT NULL, - allocations_count integer NOT NULL, + charge_card_accounts_count integer NOT NULL DEFAULT 0, + payment_accounts_count integer NOT NULL DEFAULT 0, + expense_payment_types_count integer NOT NULL DEFAULT 0, + allocations_count integer NOT NULL DEFAULT 0, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, workspace_id integer NOT NULL );
🧹 Nitpick comments (1)
tests/test_sageintacct/test_utils.py (1)
2085-2085: Rename unused loop variable.The
namevariable in the loop is never used. Rename it to_to indicate it's intentionally unused.Apply this diff:
- for name, mock_path, sync_method, attr_type, model in sync_tests: + for _, mock_path, sync_method, attr_type, model in sync_tests:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/internal/migrations/0026_auto_generated_sql.py(1 hunks)apps/sage_intacct/models.py(2 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(19 hunks)tests/test_sageintacct/test_utils.py(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/sage_intacct/models.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_sageintacct/test_utils.py (2)
apps/sage_intacct/models.py (1)
SageIntacctAttributesCount(2002-2050)apps/sage_intacct/utils.py (1)
is_sync_allowed(179-195)
🪛 Ruff (0.14.5)
tests/test_sageintacct/test_utils.py
2022-2022: Unused function argument: db
(ARG001)
2043-2043: Unused function argument: db
(ARG001)
2064-2064: Unused function argument: db
(ARG001)
2085-2085: Loop control variable name not used within loop body
Rename unused name to _name
(B007)
2105-2105: Unused function argument: db
(ARG001)
2118-2118: Unused function argument: field
(ARG001)
2118-2118: Unused function argument: value
(ARG001)
2118-2118: Unused function argument: updated_at
(ARG001)
2121-2121: Unused function argument: field
(ARG001)
2132-2132: Unused function argument: db
(ARG001)
2132-2132: Unused function argument: create_dependent_field_setting
(ARG001)
apps/internal/migrations/0026_auto_generated_sql.py
6-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-53: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (6)
tests/test_sageintacct/test_utils.py (4)
27-30: LGTM! Good addition of count mocking.The added mocking for
.count()API calls correctly supports the new attribute count tracking functionality. The return values align with the test assertions.Also applies to: 78-81, 161-164, 187-190, 319-322, 1490-1493
2022-2061: LGTM! Comprehensive test coverage for sync limits.The tests correctly verify the
is_sync_allowedlogic for both standard attributes (30K limit) and cost types (500K limit), including the workspace creation date threshold behavior.
2105-2130: LGTM! Proper test of allocation sync with timestamp.The test correctly verifies that allocation sync respects the
latest_updated_atparameter and persists the count toSageIntacctAttributesCount.
2132-2224: LGTM! Excellent comprehensive test of count persistence.This test thoroughly verifies that all sync methods correctly persist their attribute counts to
SageIntacctAttributesCount. The sequential structure withrefresh_from_db()ensures each count update is properly saved.apps/internal/migrations/0026_auto_generated_sql.py (1)
13-50: LGTM! Idempotent backfill logic.The forward SQL correctly uses
NOT EXISTSto ensure idempotent backfilling of attribute counts for existing workspaces. All fields are properly initialized.tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
3428-3468: intacct_sync_timestamps schema looks good.The table definition for
intacct_sync_timestampsis well-structured: all optional synced_at columns are nullable (allowing partial sync states), required audit fields (created_at, updated_at) are NOT NULL, workspace_id is properly constrained, and the GENERATED BY DEFAULT AS IDENTITY sequence is correctly configured.
| WHERE sic.workspace_id = w.id | ||
| ); | ||
| """, | ||
| reverse_sql=""" DELETE FROM sage_intacct_attributes_count WHERE EXISTS ( SELECT 1 FROM sage_intacct_attributes_count WHERE workspace_id = sage_intacct_attributes_count.workspace_id ); """ |
There was a problem hiding this comment.
Fix reverse SQL - will delete all rows.
The WHERE EXISTS clause will always evaluate to true for every row in the table, causing the reverse migration to delete all attribute count records, not just the backfilled ones.
Since distinguishing backfilled rows from organically created rows is not feasible, make this migration non-reversible.
Apply this diff:
- reverse_sql=""" DELETE FROM sage_intacct_attributes_count WHERE EXISTS ( SELECT 1 FROM sage_intacct_attributes_count WHERE workspace_id = sage_intacct_attributes_count.workspace_id ); """
+ reverse_sql=migrations.RunSQL.noop🤖 Prompt for AI Agents
In apps/internal/migrations/0026_auto_generated_sql.py around line 51, the
provided reverse SQL uses a WHERE EXISTS that will match every row and thus
delete all records; make this migration non-reversible instead of providing a
broken reverse SQL. Replace the current reverse_sql value with a non-reversible
marker (use Django's no-op reverse for RunSQL or set reverse_sql to None) and
add a short comment explaining that the reverse is intentionally omitted because
backfilled rows cannot be distinguished from real rows.
| COPY public.sage_intacct_attributes_count (id, accounts_count, items_count, vendors_count, employees_count, departments_count, classes_count, customers_count, projects_count, locations_count, expense_types_count, tax_details_count, cost_codes_count, cost_types_count, user_defined_dimensions_details, charge_card_accounts_count, payment_accounts_count, expense_payment_types_count, allocations_count, created_at, updated_at, workspace_id) FROM stdin; | ||
| 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2022-09-20 08:38:48.66191+00 2022-09-20 08:38:48.661952+00 1 | ||
| \. |
There was a problem hiding this comment.
Fix invalid JSON value in jsonb column.
The COPY statement for sage_intacct_attributes_count contains invalid JSON in the user_defined_dimensions_details column. Line 10266 shows value 0, but this column is jsonb type (line 3821 of hunk 4). Valid JSON requires either \N (NULL), '{}' (empty object), or '[]' (empty array). The value 0 will fail JSON parsing.
Apply this diff to use NULL instead of 0:
-COPY public.sage_intacct_attributes_count (id, accounts_count, items_count, vendors_count, employees_count, departments_count, classes_count, customers_count, projects_count, locations_count, expense_types_count, tax_details_count, cost_codes_count, cost_types_count, user_defined_dimensions_details, charge_card_accounts_count, payment_accounts_count, expense_payment_types_count, allocations_count, created_at, updated_at, workspace_id) FROM stdin;
-1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2022-09-20 08:38:48.66191+00 2022-09-20 08:38:48.661952+00 1
+COPY public.sage_intacct_attributes_count (id, accounts_count, items_count, vendors_count, employees_count, departments_count, classes_count, customers_count, projects_count, locations_count, expense_types_count, tax_details_count, cost_codes_count, cost_types_count, user_defined_dimensions_details, charge_card_accounts_count, payment_accounts_count, expense_payment_types_count, allocations_count, created_at, updated_at, workspace_id) FROM stdin;
-1 0 0 0 0 0 0 0 0 0 0 0 0 0 \N 0 0 0 0 2022-09-20 08:38:48.66191+00 2022-09-20 08:38:48.661952+00 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| COPY public.sage_intacct_attributes_count (id, accounts_count, items_count, vendors_count, employees_count, departments_count, classes_count, customers_count, projects_count, locations_count, expense_types_count, tax_details_count, cost_codes_count, cost_types_count, user_defined_dimensions_details, charge_card_accounts_count, payment_accounts_count, expense_payment_types_count, allocations_count, created_at, updated_at, workspace_id) FROM stdin; | |
| 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2022-09-20 08:38:48.66191+00 2022-09-20 08:38:48.661952+00 1 | |
| \. | |
| COPY public.sage_intacct_attributes_count (id, accounts_count, items_count, vendors_count, employees_count, departments_count, classes_count, customers_count, projects_count, locations_count, expense_types_count, tax_details_count, cost_codes_count, cost_types_count, user_defined_dimensions_details, charge_card_accounts_count, payment_accounts_count, expense_payment_types_count, allocations_count, created_at, updated_at, workspace_id) FROM stdin; | |
| 1 0 0 0 0 0 0 0 0 0 0 0 0 0 \N 0 0 0 0 2022-09-20 08:38:48.66191+00 2022-09-20 08:38:48.661952+00 1 | |
| \. |
🤖 Prompt for AI Agents
In tests/sql_fixtures/reset_db_fixtures/reset_db.sql around lines 10265-10267,
the COPY data row uses plain 0 for the jsonb column
user_defined_dimensions_details (column position matching hunk 4 line 3821),
which is invalid JSON; replace that 0 with the COPY NULL marker \N (or a valid
JSON literal like '{}' or '[]') so the row parses correctly and the column count
remains unchanged.
3 participants
Description
feat: add support for intacct attribute count api
Clickup
https://app.clickup.com/
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.