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

Business Search to tabbed interface #90

Merged
merged 6 commits into from
Jul 7, 2022
Merged

Conversation

lmallika86
Copy link

Issue #: /bcgov/entity#12641

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 PPR license (Apache 2.0).

@lmallika86 lmallika86 requested a review from kialj876 July 5, 2022 22:28
@lmallika86 lmallika86 self-assigned this Jul 5, 2022
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #90 (404a589) into main (9969d52) will increase coverage by 0.60%.
The diff coverage is 74.41%.

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   70.27%   70.87%   +0.60%     
==========================================
  Files          52       54       +2     
  Lines        1497     1631     +134     
  Branches      168      193      +25     
==========================================
+ Hits         1052     1156     +104     
- Misses        415      437      +22     
- Partials       30       38       +8     
Flag Coverage Δ
search-api 70.87% <74.41%> (+0.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ources/v1/businesses/documents/document_request.py 81.48% <ø> (ø)
...i/src/search_api/resources/v1/purchase_requests.py 63.33% <63.33%> (ø)
search-api/src/search_api/config.py 84.69% <100.00%> (+0.48%) ⬆️
...i/src/search_api/models/document_access_request.py 90.00% <100.00%> (+0.58%) ⬆️
...equest_handlers/document_access_request_handler.py 92.30% <100.00%> (ø)
search-api/src/search_api/resources/__init__.py 100.00% <100.00%> (ø)
search-api/src/search_api/resources/v1/__init__.py 100.00% <100.00%> (ø)
...i/src/search_api/resources/v1/businesses/search.py 73.17% <0.00%> (-7.07%) ⬇️
...ch-api/src/search_api/request_handlers/__init__.py 100.00% <0.00%> (ø)
...arch-api/src/search_api/request_handlers/search.py 87.30% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4af93ff...404a589. Read the comment docs.

Copy link
Collaborator

@kialj876 kialj876 left a comment

Choose a reason for hiding this comment

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

Looks good! Just has a test failure and can you add a postman test + example for the endpoint?

@@ -119,6 +119,10 @@ class _Config(): # pylint: disable=too-few-public-methods
except (TypeError, ValueError):
JWT_OIDC_JWKS_CACHE_TIMEOUT = 300

JWT_OIDC_USERNAME = os.getenv('JWT_OIDC_USERNAME', 'username')
JWT_OIDC_FIRSTNAME = os.getenv('JWT_OIDC_FIRSTNAME', 'firstname')
JWT_OIDC_LASTNAME = os.getenv('JWT_OIDC_LASTNAME', 'lastname')
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are these used for? Were they left in by mistake?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, This is the reason why user details were not getting saved in the db


const { search } = useSearch()

const totalResultsLength = computed(() => search.results?.length || 0 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

fyi this will actually be the wrong value if the number of results is greater than the rows specified. The total will be here: search.searchResults.total_rows
(this is an existing bug)

Copy link
Author

Choose a reason for hiding this comment

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

Oh but what is the expected behavior in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well I would have it show the full total since we'll be adding paging in

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I guess that can wait until we implement paging on the front end

Lekshmi Mallika added 2 commits July 6, 2022 14:58
@lmallika86
Copy link
Author

Looks good! Just has a test failure and can you add a postman test + example for the endpoint?

Test is fixed. I will add postman test in another PR

@lmallika86 lmallika86 merged commit 15d8b41 into bcgov:main Jul 7, 2022
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.

2 participants