-
Notifications
You must be signed in to change notification settings - Fork 40
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
added corp type and filing type to fee schedules endpoint #762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge once test cases are added. Also please see the comments.
CorpType.code == FeeSchedule.corp_type_code). \ | ||
join(FilingType, FilingType.code == FeeSchedule.filing_type_code) | ||
query = query.filter( | ||
or_(func.lower(FilingType.description).contains(description_list.lower(), autoescape=True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be using like
rather than contains
? with autoescape can contains
handle multiple %
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm..yeah..i am planning to rewrite this... i was trying to split the string and do a like in both of the fields ; i think thats the way to go...the challenge with multiple % is basically we are searching same string in two columns ...might not be present in one column ;but in other... so have to figure it out... I will definitely rewrite it after the core functionality of payment is done..
@@ -91,6 +92,16 @@ def find_all(cls, corp_type_code: str = None, filing_type_code: str = None): | |||
if corp_type_code: | |||
query = query.filter_by(corp_type_code=corp_type_code) | |||
|
|||
if description: | |||
# description is free text search | |||
description_list = description.replace(' ', '%') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be have a better name than _list
, I thought it's a list and was wondering how `lower()' function would work on list. Then realized it's not list but a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
@@ -19,12 +19,9 @@ | |||
|
|||
from pay_api.exceptions import BusinessException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added test case
Kudos, SonarCloud Quality Gate passed! |
Issue #:
bcgov/entity#8631
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).