Revert "Enable passing identifiers to ActionNetwork upsert_person()

[austinweisgrau]

This reverts most of commit 77ead60 / PR #861

I got more details on the consequences of using an identifier that is not globally unique in ActionNetwork, and was warned by ActionNetwork support that the consequences can be severe. In a more intense way than is mentioned in the official documentation, I was strongly warned against using custom identifiers.

In the PR we merged, documentation on the identifiers feature reports that if an identifier is used that is not globally unique in ActionNetwork, it is simply ignored. However, AN support told me that updates may be made to both the new resource and whatever resource already exists in ActionNetwork that holds that identifier. This behavior may cause updates that violate expectations and it won't be transparent or reported back to the developer that those updates were made. Many users who use identifiers encounter issues of this kind, and for these reasons AN support strongly encourage folks not to use identifiers unless they really have to.

At WFP we are going to be retiring our use of identifiers, because of this feedback and we don't really need them. In general, it does make sense to me that this feature should, by design, be hard to use, so I think we should actually remove this feature from the ActionNetwork connector.

A note is added to the upsert_person() method explaining why identifiers are not included as an option in case a future developer starts to go down the same rabbit hole I have just traversed.

[sharinetmc]

LGTM! Thank you for adding this, will def be super useful for future users!

[sharinetmc]

Actually, upon thinking about this further, I'm wondering if if this perhaps could be a breaking change due to the fact that perhaps other users may be using this param.

this could perhaps be a breaking change

[austinweisgrau]

We just added the identifier param very recently in #861

…

On Thu, Sep 14, 2023, 4:57 PM sharinetmc ***@***.***> wrote: ***@***.**** commented on this pull request. Actually, upon thinking about this further, I'm wondering if if this perhaps could be a breaking change due to the fact that perhaps other users may be using this param. — Reply to this email directly, view it on GitHub <#876 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO74QHT3N4H76K3ERIHMXTDX2MEOFANCNFSM6AAAAAA3ZEOJLY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[sharinetmc]

@austinweisgrau, good point, but I also did see that Ian has used the param for some recent parsons changes. @shaunagm, would you know what the best practices are for instances like this? Should I mark this as a breaking change and/or change the code Ian wrote or create an issue?

[shaunagm]

Let's tag @IanRFerguson and see if reverting will cause any complications for him? Otherwise let's go ahead and revert.

This was included in the last release, so we definitely want highlight it in the release notes for the release @KasiaHinkson is making next week. I'm not sure if it makes sense to release another major version just for this, though, or whether to keep it a minor release and just message very clearly that we are removing this thing. @Jason94, any thoughts?

[IanRFerguson]

Let's tag @IanRFerguson and see if reverting will cause any complications for him? Otherwise let's go ahead and revert.

This was included in the last release, so we definitely want highlight it in the release notes for the release @KasiaHinkson is making next week. I'm not sure if it makes sense to release another major version just for this, though, or whether to keep it a minor release and just message very clearly that we are removing this thing. @Jason94, any thoughts?

thanks for checking y'all - is this going to revert changes to Parsons globally, or just for the ActionNetwork connector? if the latter is true then by all means