search

move-coop / parsons

A python library of connectors for the progressive community.
https://www.parsonsproject.org/
Other
258 stars 128 forks source link

Revert "Enable passing `identifiers` to ActionNetwork `upsert_person()` #876

Closed austinweisgrau closed 11 months ago

austinweisgrau commented 12 months ago

This reverts most of commit 77ead6079ee03c399bbc83314351bc719ed457c3 / 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.

austinweisgrau commented 11 months ago

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 https://github.com/move-coop/parsons/pull/876#pullrequestreview-1626962657, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO74QHT3N4H76K3ERIHMXTDX2MEOFANCNFSM6AAAAAA3ZEOJLY . You are receiving this because you authored the thread.Message ID: @.***>

sharinetmc commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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