[HACK Week] Address map search in order details: analytics, enable feature flag, unit tests #15964
Conversation
Generated by 🚫 Danger |
|
|
jaclync
added
feature: order details
jaclync
changed the title
iamgabrielma
left a comment
iamgabrielma
left a comment
There was a problem hiding this comment.
Works well!
I saw a warning in the console when tapping the map button, from a quick research seems to be coming from mapkit rendering and performance, but I did not see any crash nor freezes during testing:
Tracking a LayerData with no associated Registry: Assertion with expression - layerDataSets.keys.exiting.empty() : Failed in file - /Library/Caches/com.apple.xbs/Sources/VectorKit/src/RegistryManager.mm line - 475
| updatedFields.city = "Original City" | ||
|
|
||
| // When | ||
| sut.updateFields(&updatedFields) |
There was a problem hiding this comment.
Passing the reference here caught my eye, is there a reason for the implementation to accept an inout and modify the reference rather than returning a copy? ( func updateFields(_ fields: AddressFormFields) -> AddressFormFields ). Then the tests would look something like:
// Given
fieldsToUpdate.city = "Original City"
// When
let updatedFields = sut.updateFields(fieldsToUpdate)
// Then
#expect(updatedFields.city == "Original City")
There was a problem hiding this comment.
I used inout here for direct mutation without copying the struct, which works well with SwiftUI's @Binding binding patterns. If using the functional approach (creating a new struct and updating the existing), because the map picker only updates a subset of address properties, other fields like names/email/phone need to be preserved manually with caution. Lemme know what you think!
There was a problem hiding this comment.
Interesting, thanks! The scenario where we only have to update the subset of properties while preserving the rest definitely makes for a good use case of mutating the object directly to avoid errors.
| } | ||
|
|
||
| @available(iOS 17, *) | ||
| @Test func updateFields_with_unknown_country_sets_country_and_state_strings() async { |
There was a problem hiding this comment.
Nit: I found this test name tricky to understand at first (same with the one below), as the country is known as per the mock address, and we're testing the selected fields. Perhaps adding/updating something like unmatched_countryByCode or when_country_not_found_in_countryByCode bits would clarify it
There was a problem hiding this comment.
Good point, the test names could use some improvements as you suggested. Updated in 18f3811
Thanks for testing and noticing this, which device type and iOS version did you see this warning? I tested 2 simulators and a device (all iOS 18) and didn't see this warning. I was wondering if it's specific to iOS 17. |
It could be the iOS version indeed, my physical device is iPhone 14 running 17.7.1. I'm not positive I saw this error on the 18+ simulator 👍 |
I tried with an iOS 17.4 iPad simulator and saw a bunch of |
For WOOMOB-943
Just one review is required.
Description
This PR adds analytics tracking for address map picker interactions in the order details edit flow. Two new events are tracked to understand merchant engagement with the map-based address picker feature:
order_detail_edit_address_map_picker_tappedwithlocaleproperty - Triggered when merchants tap the CTA to open the address map pickerorder_detail_edit_address_map_picker_use_address_tapped- Triggered when merchants select an address from the map pickerAdditionally, this PR includes the following changes to enable for release:
Steps to reproduce
order_detail_edit_address_map_picker_tappedevent should be tracked with alocalepropertyorder_detail_edit_address_map_picker_use_address_tappedevent should be trackedRELEASE-NOTES.txtif necessary.