Skip to content

feat: Filter transactions with no client id #77

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2025
Merged

feat: Filter transactions with no client id #77

merged 1 commit into from
Jul 11, 2025

Conversation

kofimokome
Copy link
Collaborator

No description provided.

@sourceant SourceAnt
Copy link

sourceant bot commented Jul 11, 2025 •
edited
Loading

Code Review Summary

✨ This pull request introduces a new no_client_id query parameter across several API endpoints (Category, Group, Party, Transaction, Wallet). This parameter allows filtering results to include only records where client_generated_id in their syncState is NULL. The change includes defining the OpenAPI parameter, integrating it into controller definitions, and implementing the filtering logic within the ApiQueryable trait.

🚀 Key Improvements

  • Centralized Parameter Definition: The noClientIdParam is defined once in app/Http/Controllers/Controller.php, promoting reusability and maintainability across multiple API endpoints.
  • Consistent API Filtering: The new no_client_id filter is consistently applied via the ApiQueryable trait, ensuring uniform behavior for different resources.

💡 Minor Suggestions

  • Remove extraneous blank lines after the noClientIdParam definition in app/Http/Controllers/API/v1/CategoryController.php, app/Http/Controllers/API/v1/TransactionController.php, and app/Http/Controllers/API/v1/WalletController.php for better code style consistency.

🚨 Critical Issues

  • Boolean Parameter Handling: The request()->has('no_client_id') check in app/Http/Traits/ApiQueryable.php should be updated to request()->boolean('no_client_id') to correctly handle boolean values as per the OpenAPI schema definition. This ensures the filter is only applied when the parameter is explicitly true.
  • Implicit Relationship Dependency: Ensure that any model utilizing the ApiQueryable trait is guaranteed to have a syncState relationship to prevent runtime errors from the whereHas('syncState', ...) clause. If not, this trait should be more specific or include checks for relationship existence.

sourceant[bot]
sourceant bot suggested changes Jul 11, 2025
View reviewed changes
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

@kofimokome kofimokome force-pushed the feat/get-items-with-no-client-id branch from 92b3b3f to 302f004 Compare July 11, 2025 18:25
sourceant[bot]
sourceant bot suggested changes Jul 11, 2025
View reviewed changes
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

@kofimokome kofimokome force-pushed the feat/get-items-with-no-client-id branch from 302f004 to e4d382a Compare July 11, 2025 18:27
sourceant[bot]
sourceant bot approved these changes Jul 11, 2025
View reviewed changes
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

@kofimokome kofimokome requested a review from nfebe July 11, 2025 18:28
sourceant[bot]
sourceant bot approved these changes Jul 11, 2025
View reviewed changes
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

nfebe
nfebe reviewed Jul 11, 2025
View reviewed changes
Copy link
Contributor

@nfebe nfebe left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Thank you!

app/Http/Traits/ApiQueryable.php
@@ -25,6 +25,11 @@ protected function applyApiQuery(Request $request, Builder|Relation $query): arr
throw new \InvalidArgumentException('Invalid date format for synced_since parameter.');
}
}
if ($request->has('no_client_id')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kofimokome How did you enjoy doing this in one place? 😆

@nfebe nfebe merged commit edd7c5b into main Jul 11, 2025
2 checks passed
@nfebe nfebe deleted the feat/get-items-with-no-client-id branch July 11, 2025 19:54
sourceant[bot]
sourceant bot suggested changes Jul 11, 2025
View reviewed changes
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

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

@sourceant sourceant[bot] sourceant[bot] requested changes

@nfebe nfebe nfebe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kofimokome @nfebe