-
Notifications
You must be signed in to change notification settings - Fork 81
30071 updates to business summary for bc companies #3762
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
30071 updates to business summary for bc companies #3762
Conversation
…th user/account context
…usiness summary visibility
| if status_code == HTTPStatus.OK: | ||
| return Response( | ||
| pdf_content, | ||
| mimetype='application/pdf', | ||
| headers={ | ||
| 'Content-Type': 'application/pdf', | ||
| } | ||
| ) | ||
| return pdf_content, status_code |
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.
we'll want to make sure these additions don't break the FE. Did the previous code not return the mime type of application/pdf? was that the issue you were trying to fix here?
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.
as per our conversation, let's roll this in and test if FE works properly as soon as it gets into dev
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.
Yeah, previous code was returning text/html as mime type so it was messing up the tools like postman etc ... Will verify all is good when its deployed in dev
|
can we add the new FF to |
| # if flag is not enabled return from the function | ||
| return | ||
|
|
||
| parties_json = get_parties(self._business.identifier).json['parties'] |
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.
not really ideal that we are calling the flask route function directly but maybe something we look at refactoring after this initial PR makes it in. I would like get things in so I can start testing out the LD targetting.
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.
fair enough, I just reused the code from the code above (line 328), its been called in the same way. I agree its good candidate for refactor and to move most of the logic to the service.
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.
Looks good to me, but can you add some tests please? I think reports have quite a few existing ones you can add to
Added tests, was not able to extend existing ones, but added a new ones testing the methods I have added. |
Issue #:30071 /bcgov/entity#30071
Description of changes:
updating pyproject.toml so that it can be installed on windows (missing binaries for windows installers from some package versions)* revertedBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the lear license (Apache 2.0).