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

21536 - Passing short Name Id parameter to find refunds #1783

Merged
merged 13 commits into from
Oct 18, 2024
Merged

Conversation

rodrigo-barraza
Copy link
Collaborator

Issue #:
bcgov/entity#21536

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-pay license (Apache 2.0).

@@ -80,7 +80,7 @@ def create_shortname_refund(request: dict, **kwargs):
@staticmethod
def get_shortname_refunds(data: EFTShortNameRefundGetRequest):
"""Get all refunds."""
refunds = EFTRefundModel.find_refunds(data.statuses)
refunds = EFTRefundModel.find_refunds(data.statuses, data.short_name_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update tests for short_name_id search please

"status_filter_multiple",
2,
),
(
f"?status={EFTShortnameRefundStatus.DECLINED.value}",
f"?statuses={EFTShortnameRefundStatus.DECLINED.value}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

additional parameters for short_name_id search?

input_string = data.get("status", "")
input_string = data.get("statuses", "")
short_name_id = None
if data.get("shortNameId", None) is not None:
Copy link
Collaborator

@seeker25 seeker25 Oct 18, 2024

Choose a reason for hiding this comment

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

This defeats the purpose of using the serializer or converter.. please use Serializable super().from_dict(data) instead of doing this, the statuses were a work around because the serialization needed a bit more looking at

Copy link
Collaborator

Choose a reason for hiding this comment

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

EG
image

Copy link
Collaborator

@seeker25 seeker25 Oct 18, 2024

Choose a reason for hiding this comment

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

we eventually want to get rid of dto.state = dto.state.split(",") etc somehow in the converter
same with the account_id_list shouldn't need to do this by hand.. eventually we'd like to do this all in the serializer in a generic way - so were not repeating a bunch of boilerplate code over and over again

Copy link
Collaborator

@seeker25 seeker25 Oct 18, 2024

Choose a reason for hiding this comment

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

I'll see if we can put in:

typical register structure unstructure hook - new type DelimitedString - so we wont need to do above hopefully and it's done in one spot in code

Copy link
Collaborator

@seeker25 seeker25 left a comment

Choose a reason for hiding this comment

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

comment above

Copy link
Collaborator

@seeker25 seeker25 left a comment

Choose a reason for hiding this comment

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

perfect thank you

@seeker25 seeker25 merged commit 0723ede into main Oct 18, 2024
18 checks passed
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.

3 participants