Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SKU to Item #379

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add SKU to Item #379

wants to merge 4 commits into from

Conversation

laf-rge
Copy link
Contributor

@laf-rge laf-rge commented Mar 16, 2025

Fix SKU Field Retrieval and Account Test Issues (Fixes #373)

Changes

This PR addresses two main issues:

  1. SKU Field Retrieval Fix
  • Fixed issue where SKU field was not being returned in Item.all() queries
  • Added test case test_sku_in_all to verify SKU field retrieval
  • Removed trailing slashes from API URLs to ensure consistent behavior
  1. Account Test Improvements
  • Enhanced account test reliability by using shorter, unique identifiers
  • Improved test cleanup and isolation
  • Fixed test failures related to duplicate account names/numbers

Technical Details

SKU Field Fix

  • Modified URL construction in get_single_object to remove trailing slashes
  • Added explicit test case to verify SKU field is properly returned in list queries
  • Verified SKU field retrieval works in both individual and bulk item queries

Account Test Improvements

  • Implemented shorter timestamp format for account identifiers to stay within QB's 20-char limit
  • Enhanced test isolation by using unique identifiers for each test run
  • Removed unnecessary sleep statements that were masking the real issues

Testing

  • All tests are now passing, including:
    • Item tests (including new SKU field test)
    • Account integration tests
    • Unit tests for URL handling
  • Verified that SKU field is properly returned in production scenarios
  • Confirmed account creation/updates work reliably with new identifier format

Breaking Changes

None. This is a backward-compatible fix that improves existing functionality.

Related Issues

  • Fixes issue where SKU field was missing from Item.all() results
  • Resolves account test failures due to identifier length and uniqueness

Additional Notes

  • The URL trailing slash fix also improves consistency across all API calls
  • Account test improvements make the test suite more reliable and faster

laf-rge added 4 commits March 16, 2025 12:25
The SKU field was not being properly returned when using Item.all() due to
the field not being explicitly requested in the query. This commit:

1. Adds test_sku_in_all() to tests/integration/test_item.py to verify SKU
   field retrieval
2. Removes trailing slashes from API URLs in get_single_object method
3. Cleans up test files by:
   - Removing sleep statements and debug prints from test_account.py
   - Using proper mocking in unit tests to avoid session dependency
   - Making test names and assertions more descriptive

The changes ensure that SKU values are correctly returned when querying
items through the all() method, while also improving the overall test
suite maintainability.

Testing:
- Added new integration test verifying SKU field retrieval
- All existing Item and Account tests pass
- Unit tests properly mock session dependencies
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 72.72727% with 9 lines in your changes missing coverage. Please review.

Project coverage is 93.34%. Comparing base (67141c2) to head (67e94c2).

Files with missing lines Patch % Lines
quickbooks/mixins.py 41.66% 3 Missing and 4 partials ⚠️
tests/unit/test_client.py 75.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
- Coverage   93.53%   93.34%   -0.20%     
==========================================
  Files         104      104              
  Lines        3991     3994       +3     
  Branches      108      105       -3     
==========================================
- Hits         3733     3728       -5     
- Misses        219      221       +2     
- Partials       39       45       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@laf-rge
Copy link
Contributor Author

laf-rge commented Mar 16, 2025

Got annoyed with the broken tests so this fixes that as well. If you want me to break that out I can but it is straightforward. (drops 2.x mock external dependency and mocks the auth_client)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Items return from Item.all() SKU property always None
2 participants