chore(queries): Remove constructor override to use Filters with validation #4588
Conversation
| def wallet | ||
| @wallet ||= organization.wallets.find_by(id: filters.wallet_id) | ||
| end |
There was a problem hiding this comment.
Maybe I should remove this.
then, use organization.where(id: ...).exists?to return not_found_failure and add.where(wallet_id: ...)` to the base scope 🤔
There was a problem hiding this comment.
it is clear as it is.
however if we can drop the not_found_failure on wallet and use a base scope like we do for the webhooks query is also good. All in all the wallet is a filter, if wallet is not found no wallet transaction would be returned. However this could be a breaking change on public APIs
There was a problem hiding this comment.
Yeah, I'd prefer to do it this way but I definitely don't want to introduce a breaking change
ancorcruz
left a comment
ancorcruz
left a comment
There was a problem hiding this comment.
really nice refactor. looks good to me.
| rescue BaseService::FailedResult | ||
| result |
There was a problem hiding this comment.
what is being rescued here?
There was a problem hiding this comment.
I think it's because the validate_filters raise a validation failure. So we could argue the return result unless validate.success? is the "useless" part but it's done this way in other query objects so I want to follow the pattern for now.
I think we should definitely improve the validation part. Like make the BaseQuery ensure it has validation. We'll see
| def wallet | ||
| @wallet ||= organization.wallets.find_by(id: filters.wallet_id) | ||
| end |
There was a problem hiding this comment.
it is clear as it is.
however if we can drop the not_found_failure on wallet and use a base scope like we do for the webhooks query is also good. All in all the wallet is a filter, if wallet is not found no wallet transaction would be returned. However this could be a breaking change on public APIs
| rescue BaseService::FailedResult | ||
| result |
There was a problem hiding this comment.
same question, what is being rescued here... I haven't seen this rescue in other query objects.
There was a problem hiding this comment.
the majority of objects don't have contracts validation
I was looking at the pagination and realize that 2 Query object (only 2 out of ~30) have an override of the constructor.
I think it's better to rely on filters of this and let Dry::Validation gem handle the required/optional filters.
The main benefits are: