2765 Fix geocoding to display enterprises on map #2872
Conversation
Enterprises have an `address_id` which must point to a valid `Spree::Address`. As Rubocop suggested, I restricted the deletion of addresses when they are still associated to an enterprise. Without declaring `dependent: :restrict`, trying to delete the address would raise `ActiveRecord::InvalidForeignKey`. Now it is more specific and raises `ActiveRecord::DeleteRestrictionError`. I didn't find code rescuing the InvalidForeignKey when deleting addresses. I actually think that we never delete addresses. So this change should not have any impact on the execution.
|
As this is quite urgent and I'm confident that it won't break anything, I deployed this to Australian production and our map works again. Every enterprise that is not in the right location needs to be updated in the admin interface. Changing the address will trigger the geocoding so that it appears on the map. |
|
Staged on https://staging.openfoodnetwork.org.au/. I configured the API key for use. I know that this is still in code review, but I wanted to test it to make sure that it works. A possible addition would be a migration that geocodes all enterprises. But I think that it would be quite costly to geocode thousands of enterprises while there are only a few that need to be geocoded. While we could geocode only enterprises that don't have a location yet, we don't know which enterprises changed address and are displayed in the wrong location. |
sigmundpetersen
requested review from
sauloperez,
luisramos0 and
Matt-Yorkley
| @@ -1,5 +1,5 @@ | |||
| Spree::Address.class_eval do | |||
| has_one :enterprise | |||
| has_one :enterprise, dependent: :restrict | |||
| belongs_to :country, class_name: "Spree::Country" | |||
There was a problem hiding this comment.
This needs a test. Could be a simple one trying to delete an address used in an enterprise.
There was a problem hiding this comment.
Hm, I think I agree that a unit test could be useful to validate that this Rails magic works as intended. But I hesitated because we are not deleting address anywhere in the code. So if it's not a requirement, if it's not a specification to delete an address, why should it be added to the specs? I'll still do it for code coverage.
There was a problem hiding this comment.
cool, thanks!
imo, if it's worth a change in the code, it's worth a test. the only case I would skip the test is if it is too difficult to test for the value it adds, which is not the case. this is only my opinion :-)
| # their way into the database. I don't know what the source of them is, so this patch | ||
| # is designed to track them down. | ||
| # This is intended to be a temporary investigative measure, and should be removed from the | ||
| # code base shortly. If it's past 17-10-2013, take it out. |
There was a problem hiding this comment.
it's exactly 5 years later!!!
| geocode_address = [address1, address2, zipcode, city, country.andand.name, state.andand.name] | ||
| filtered_address = geocode_address.select{ |field| !field.nil? && field != '' } | ||
| filtered_address.compact.join(', ') | ||
| render_address([address1, address2, zipcode, city, country.andand.name, state.andand.name]) |
There was a problem hiding this comment.
these methods should be in a separate service, some sort of geocoder_service that formats addresses according to different given inputs. And this service methods should be unit tested at least with simple positive examples.
another option, is to add functional tests to avoid this bug from re-appearing without us noticing.
There was a problem hiding this comment.
I agree that these methods would be better in a separate service. But I didn't want to go too far out of scope. All commits except the last one with the initializer are pure refactoring and don't contribute anything to fixing the original issue. It was just me understanding the code and cleaning it up along the way. I thought about not including them in the PR, but then I thought it might be useful to others to know where the geocoding is actually happening.
Before we pull the rendering out into a service, we should discuss using our own model and table for enterprise addresses. We have some ugly hacks to use Spree::Address for enterprise locations. For example, Spree::Address requires first and last name, but enterprises don't have that. So in the enterprise model we set these fields to "unused" to make the addresses valid.
We also added the has_one :enterprise relationship which adds unneeded bulk to all address objects that are used for orders. All addresses for orders get automatically geolocated even though we never use that. Point is: enterprise addresses have a very different purpose and hold different data to Spree's addresses.
By the way, the rendering method is unit tested:
https://github.com/openfoodfoundation/openfoodnetwork/blob/8fb81bb6a7da16f8ca0124b448e5315aa5c8fba2/spec/models/spree/addresses_spec.rb
But that wasn't the problem. The problem was that Google decided to give different responses to API requests. The only way to catch that is a live test. Usually all specs are set up to not use an internet connection. External services are mocked or HTTP responses are recorded and replayed. Hitting a live API is very bad practice in tests. The correct HTTP response depends on the API key, the setup in the developer console, the availability of a valid credit card and Google's terms of service. I'm not sure we can test that in a reasonable way.
There was a problem hiding this comment.
ahaha, I meant unit tests on the rendering methods only! the tests were there already. sorry about that.
the full refactor of address is a more complicated issue, agree. thanks for the details!
I think we could still just extract these methods to a geocoding service separate from the address class but fully appreciate what you said about refactoring. it's important we feel encouraged to refactor but don't feel pressed to make it perfect. a little improvement is always very welcome!
|
@sauloperez @luisramos0 can we still test or do the changes you ask need to be solved and then the PR restaged? I thought we shouldn't stage before approved, but here it's a bit urgent, so I would like a clarification: can we test? |
|
my request for change is to add tests, the code will probably not change, so it's fine if you continue with manual testing. |
|
I just tested on Aus staging but this is not working, new enterprises are not geolocalised, so of course modification doesn't work either :-( Not sure Maikel works tomorrow, so I don't know @sauloperez you proposed your help if you have time to work on it? I don't see what I can do wrong, I'm juste creating an enteprise... |
|
in this context, I bet it's a setup problem. above @mkllnk says he configured and successfully tested in prod. @mkllnk doesnt say he validated in staging. maybe there's a problem in staging setup. also the live validation was about changing, not creating. do you want to test that @myriamboure ? maybe creating is also broken in aus live. |
|
I can pick it up tomorrow, yes. |
|
@luisramos0 as you suggested I tested on Australian production and it works properly both on creation and modification, testing notes: https://docs.google.com/document/d/1rvQTVf3uEvpDsx-CAJasYz-A9d3w0hxqer_xSbZ9tjM/edit# So it seems to be some staging configuration issue. If it is deployed on Aus production and working maybe we can consider tests are passing, no? Can someone do the thing for French production setup about the Google API thing (@pacodelaluna or @luisramos0 or @sauloperez ) so that once this is merged we can upgrade to master and do our presentation on thursday? |
|
There are things to change for the reviewers to be happy before merging anyway, but then maybe we should consider as tested and ready to go... @sauloperez if you pick it up tomorrow as Maikel is not around this Wed. tell me if that sounds good to you. |
|
"If it is deployed on Aus production and working maybe we can consider tests are passing, no?" @myriamboure there are no automated tests. |
|
This is the feature that will break again in the future without us noticing, isnt it? That's why tests are so important here. |
|
@luisramos0 Yes, this PR would unblock #2738. I'm surprised this didn't work on Australian staging as I tested it there as well. Maybe it's a quota problem. I will see. |
|
@myriamboure I just tested on https://staging.openfoodnetwork.org.au/map and it worked without a problem. I can change the address of existing enterprises and I can create new enterprises and they are shown on the map. Can you add some testing notes about what you did and what didn't work? |
This conversion is done by Transpec 3.3.0 with the following command:
transpec spec/models/spree/addresses_spec.rb
* 13 conversions
from: obj.should
to: expect(obj).to
* 2 conversions
from: it { should ... }
to: it { is_expected.to ... }
* 1 conversion
from: obj.should_not
to: expect(obj).not_to
For more details: https://github.com/yujinakayama/transpec#supported-conversions
|
@luisramos0 @sauloperez I added a test for address deletion to cover one of my refactoring commits. Initially I added that commit to satisfy rubocop. I have no test to cover the original issue, because it is entirely dependent on the live result of the Google API. The only solution would be specs to run on a live server to test with the real API key and a live connection. Maybe that could be a rake task that can be called manually? Does anyone have experiences with automated live testing? |
There was a problem hiding this comment.
ok! thanks @mkllnk
My "request changes" had 3 parts:
- test the deletion - now added
- test the render_address methods - my mistake, they were already tested. sorry!
- test the integration with the api - I think the best approach is to create a separate issue for this live test/verification, and then see how we can achieve some form of monitoring on the api integration
2 similar comments
|
@luisramos0 Follow up: https://community.openfoodnetwork.org/t/live-integration-tests-setup-verification/1464 @sauloperez Are you happy with the test? |
6 similar comments
|
@luisramos0 Follow up: https://community.openfoodnetwork.org/t/live-integration-tests-setup-verification/1464 @sauloperez Are you happy with the test? |
|
@luisramos0 Follow up: https://community.openfoodnetwork.org/t/live-integration-tests-setup-verification/1464 @sauloperez Are you happy with the test? |
|
@luisramos0 Follow up: https://community.openfoodnetwork.org/t/live-integration-tests-setup-verification/1464 @sauloperez Are you happy with the test? |
|
@luisramos0 Follow up: https://community.openfoodnetwork.org/t/live-integration-tests-setup-verification/1464 @sauloperez Are you happy with the test? |
|
@luisramos0 Follow up: https://community.openfoodnetwork.org/t/live-integration-tests-setup-verification/1464 @sauloperez Are you happy with the test? |
|
@luisramos0 Follow up: https://community.openfoodnetwork.org/t/live-integration-tests-setup-verification/1464 @sauloperez Are you happy with the test? |
|
Must have been on my side, I retested and yes it works... https://docs.google.com/document/d/1rvQTVf3uEvpDsx-CAJasYz-A9d3w0hxqer_xSbZ9tjM/edit?usp=sharing so all green on testing side ! Just waiting for @sauloperez approval and then it can be merged. |
What? Why?
Closes #2765
Google changed their terms of service recently. They require us to have an API key connected to a credit card to use their geocoding service now. This change uses the API key we already have for Google Maps.
What should we test?
This needs to be tested on a server with a valid API key which has the Geocoding API enabled.
Release notes
Updated Geocoder configuration to work with Google's new terms of service.
Changelog Category: Changed
Documentation updates
We need to update https://github.com/openfoodfoundation/ofn-install/wiki/Google-Maps to mention the Geocoding API. We should also update the Security section, because there is no valid key restriction to work with all our use cases (we can either restrict a key to a referrer or to an IP, but we need both to use the key for the map and the geocoding).