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

PIMS-307 Geocoder search in property filter #1279

Merged
merged 3 commits into from Oct 6, 2022
Merged

Conversation

Fosol
Copy link
Contributor

@Fosol Fosol commented Sep 23, 2022

Description

The property search filter will now display address results as the user types. These address results are from Geocoder. This does not include results from inventory regrettably as this would be quite a bit more work. If I can find more time next week I may be able to add it.

I had to update the packages, github action, and reduce coverage to get the build to work. The old build (14.x) doesn't seem to be able to complete.

image

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

As you type in the address input for search it will make a request to Geocoder for results. It reuses the same component used in other areas to perform the same action. When you select a value it places it in the input.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #1279 (0c8d939) into dev (df614cb) will increase coverage by 0.04%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1279      +/-   ##
==========================================
+ Coverage   59.66%   59.71%   +0.04%     
==========================================
  Files         887      888       +1     
  Lines       23643    23652       +9     
  Branches     4273     4277       +4     
==========================================
+ Hits        14106    14123      +17     
+ Misses       9147     9139       -8     
  Partials      390      390              
Flag Coverage Δ
unittests 59.71% <77.77%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
...res/properties/components/GeocoderAutoComplete.tsx 82.22% <ø> (+22.22%) ⬆️
...atures/properties/filter/PropertyFilterOptions.tsx 76.92% <66.66%> (-8.80%) ⬇️
frontend/src/components/common/form/InputGroup.tsx 94.44% <100.00%> (+0.69%) ⬆️
...operties/components/GeocoderAutoCompleteStyled.tsx 100.00% <100.00%> (ø)

@Fosol
Copy link
Contributor Author

Fosol commented Sep 24, 2022

Having issues with scss atm the moment. The build is having an known issue - facebook/create-react-app#5372

@@ -16,7 +16,7 @@ jobs:

strategy:
matrix:
node-version: [14.x]
node-version: [14.19.3]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Fosol just curious if we don't specify the node version does it create an issue with our github workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The github action was failing so I had to specify what worked locally.

Resolve build issue
Node version
Reduce coverage requirement
@Fosol
Copy link
Contributor Author

Fosol commented Sep 29, 2022

@LawrenceLau2020 I'll let you merge this when you want it. I don't have time to add additional unit tests regrettably.

@LawrenceLau2020
Copy link
Collaborator

Member

@LawrenceLau2020 I'll let you merge this when you want it. I don't have time to add additional unit tests regrettably.

@Fosol it seems we cannot merge the code unless we meet the codecov target of 60%, would we need to add new unit tests in order to meet the Codecov target of 60% to be able to merge this PR?

@Fosol
Copy link
Contributor Author

Fosol commented Sep 29, 2022

@LawrenceLau2020 Either we wait for me to find time to create additional unit tests, or we bypass the requirement. Up to you?

@LawrenceLau2020
Copy link
Collaborator

@LawrenceLau2020 Either we wait for me to find time to create additional unit tests, or we bypass the requirement. Up to you?

@Fosol sure, would you be able to write the additional unit tests within the next 4 weeks?

@Fosol
Copy link
Contributor Author

Fosol commented Oct 6, 2022

@LawrenceLau2020 I was able to add a few tests for the component I updated. It appears to be passing the coverage checks now. The only check that doesn't pass is the old build (14.x) which will disappear after this PR is merged. Let me know if this is good enough?

@LawrenceLau2020
Copy link
Collaborator

Awesome, Jeremy, looks good!

@Fosol Fosol merged commit 01771c2 into bcgov:dev Oct 6, 2022
@Fosol Fosol deleted the pims-307 branch October 6, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PIMS
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

2 participants