joshuapinter / react-native-unified-contacts

Your best friend when working with the latest and greatest Contacts Framework in iOS 9+ in React Native.
MIT License
158 stars 56 forks source link

Add ability to add and update postal addresses #40

Closed paintedbicycle closed 6 years ago

paintedbicycle commented 6 years ago

Adds postal addresses to the addContact and updateContact methods.

Note: I did not know if you also wanted to include other properties like ISOCountryCode or if those are provided by iOS

joshuapinter commented 6 years ago

This looks great, @paintedbicycle. Were you able to test it out quite thoroughly?

(At some point we'll need to add a solid test suite to this library.)

paintedbicycle commented 6 years ago

We should test a little more. I tested an update on iOS 11 but with simple data. If you can’t test it out today I’ll do a bit more testing tomorrow and ping you.

joshuapinter commented 6 years ago

Sounds good. I'm tied up with a couple other things today so I'll wait to hear from you tomorrow. Thanks!

paintedbicycle commented 6 years ago

OK I did some testing and things should be good to go.

Note that I still need an answer to my question at the start of this PR. Do you need any of the following fields added into this?

I'm trying to keep things consistent with what you're outputting here:

  "postalAddresses": [
    {
      "label": "Home",
      "city": "City",
      "state": "CA",
      "localizedLabel": "home",
      "postalCode": "98765",
      "country": "United States",
      "isoCountryCode": "",
      "stringValue": "123 St\nCity CA 98765\nUnited States",
      "street": "123 St",
      "identifier": "7A472311-AB4F-46F4-B046-B60143DBC858"
    }
  ],

But I haven't dug into how isoCountryCode, stringValue and identifier are being generated. Are they automatic or do they need to be passed?

joshuapinter commented 6 years ago

This looks good. We don't need isoCountryCode or mailingAddress now. I think the basics are all we need to start and we can add those later as required/requested.

The only one that might be necessary when updating is the identifier because that's the primary key for iOS.

Were you able to update a contact's postal address in your tests? If so, we should be good to go.

Let me know and I'll get this merged in.

paintedbicycle commented 6 years ago

I believe it worked fine with postal, yes, but you have me second guessing. I'll check out a clean copy and try again and let you know.

As for identifier, is this generated by iOS on each update? I don't want to overwrite some system setting. Or, should it be passed in again on update?

joshuapinter commented 6 years ago

Identifier is like the UID of the record. It's generated by the OS and I believe can be used to lookup an existing record.

For a postalAddress, it will have it's own identifier. I'm just not sure if you need to pass it in to update the contact's postalAddresses. For example, if the contact has multiple postalAddress's, my thinking is that you would have to pass in the identifier in order to update them, otherwise the OS wouldn't know which one(s) to update. It might create new ones and essentially duplicate the postalAddress's already there.

If you can just test that, that would be great. Again, what this library really needs is a solid test suite.

paintedbicycle commented 6 years ago

Agreed, it does need tests. I've never written any, so not sure I can help much there.

As for identifier I can have a look, but your library erases all of the pre-existing array values and then re-saves them. It doesn't actually update them.

It would be great if it did, but with the removal of old addresses and the requirement to re-pass both old and new, I don't think it would matter if iOS needs the identifier or not

joshuapinter commented 6 years ago

Okay, if it clears the existing array of postalAddresses and overwrites it with new ones then you shouldn't need to worry about the identifier for now. Perhaps later we can handle that more eloquently if a user is trying to update a single postalAddress as opposed to an entire contact record, but it's fine for now.

Are we good to merge then?

paintedbicycle commented 6 years ago

I agree, there should be a more elegant way - however I just followed your lead as this is what you do with both email addresses and telephone numbers.

I'm just installing this again now. I'll let you know in a few minutes

joshuapinter commented 6 years ago

👍

paintedbicycle commented 6 years ago

@joshuapinter Confirmed. Both new addresses and updated addresses both save postalCode if provided in the proper format. Go ahead and merge.

      updatedRecord.postalAddresses = [{
        label: 'home',
        city: 'CityName',
        state: 'CA',
        postalCode: '98765',
        country: 'United States',
        street: '123 St',
      }];
joshuapinter commented 6 years ago

👍 Excellent!