action-destinations icon indicating copy to clipboard operation
action-destinations copied to clipboard

[WIP] Update mailingLists type for Loops

Open tcgilbert opened this issue 1 year ago • 8 comments

This PR modifies the mailingLists field from an array of objects to a single object. Since users can't manually provide objects for the array, the previous structure was not very useful. It's unlikely that event payloads would include an array of mailingLists objects by default.

With this update, users can now easily input multiple mailling list IDs when creating a mapping, which will look like the following: Screenshot 2024-09-17 at 3 06 45 PM

This is more in line with the request parameters outlined in Loops API docs.

Considerations

I did run some queries to see if any users are actively making use of mailingLists with the action currently and there is one customer that is. That being the case we need to either:

  • Create a new v2 action
  • Add an additional field (one that uses array of objects, and one that is just an object)
  • Notify the customer that is making use of this field of the change they will need to put in place
    • while this option is not desirable, the use may be happy to accomodate the change as they are using a hacky work around to get the mailing lists ids that they want in the mapping
Screenshot 2024-09-17 at 4 46 22 PM Screenshot 2024-09-17 at 4 46 57 PM

Testing

Include any additional information about the testing you have completed to ensure your changes behave as expected. For a speedy review, please check any of the tasks you completed below during your testing.

  • [ ] Added unit tests for new functionality
  • [ ] Tested end-to-end using the local server
  • [ ] [Segmenters] Tested in the staging environment

tcgilbert avatar Sep 17 '24 20:09 tcgilbert

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 82.94%. Comparing base (d0f4128) to head (44474d3). Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2414      +/-   ##
==========================================
+ Coverage   82.91%   82.94%   +0.02%     
==========================================
  Files         943      944       +1     
  Lines       16437    16460      +23     
  Branches     3088     3098      +10     
==========================================
+ Hits        13629    13652      +23     
  Misses       2614     2614              
  Partials      194      194              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 17 '24 21:09 codecov[bot]

Thanks for this Thomas. We'll try it out.

How can we also test the UI you show in your screenshot? Action Tester has a different UI

danrowden avatar Sep 18 '24 10:09 danrowden

How can we also test the UI you show in your screenshot? Action Tester has a different UI

@danrowden Afraid the actions tester won't mock the production UI accurately. That said the "Custom Attributes" field is the same type, so it will look the same in the UI:

Screenshot 2024-09-18 at 7 18 18 AM

Only difference is that the defaultUI option for mailingLists will default to showing the "Edit Object" pane of the mapping

tcgilbert avatar Sep 18 '24 11:09 tcgilbert

This is what I see in Actions Tester. There is no way for me to add custom values to the right side of = here. Am I missing something in the set up to get it to work?

Screenshot 2024-09-18 at 14 52 39

danrowden avatar Sep 18 '24 11:09 danrowden

@danrowden here is a video of me inputting the key values: https://www.loom.com/share/928e1ca07771436989d466de9f5362ee?sid=b5e39c08-061e-47c6-a425-73df8a893093

if it helps happy to jump on a quick call to talk it through, lmk.

tcgilbert avatar Sep 18 '24 12:09 tcgilbert

Thanks for this! That helps

danrowden avatar Sep 18 '24 12:09 danrowden

@tcgilbert And just to confirm, users can send in mailing list data in an identify() call eg https://loops.so/docs/integrations/segment#sending-data-to-segment

For our existing customer on the current version, would this require adding a new field like mailingListData (named different), ask the customer to switch over and then remove the existing mailingLists in a future update?

danrowden avatar Sep 20 '24 09:09 danrowden

@tcgilbert And just to confirm, users can send in mailing list data in an identify() call eg https://loops.so/docs/integrations/segment#sending-data-to-segment

@danrowden yes, they could send with an identify call. If so it would likely be included in the traits object:

"traits": {
    "mailingLists": {
        "list_id_1": true
    }
}

The user would have the flexibility to map this object to Mailing lists field as they see fit with the drafted option

For our existing customer on the current version, would this require adding a new field like mailingListData (named different), ask the customer to switch over and then remove the existing mailingLists in a future update?

Yes, we'd need to leave the current field as is for now, and not remove it until we could confirm it is not in use. The other option would be to create a cloned action with the name "v2", that has the updated mailingList field as an object.

tcgilbert avatar Sep 24 '24 12:09 tcgilbert

@tcgilbert We have checked our logs and haven't found any recent activity for calls from Segment containing mailingLists data (anything with that key have an empty array). We are happy to make this change ASAP so it can roll out. We'd really like to avoid creating a new V2 action

danrowden avatar Sep 27 '24 13:09 danrowden

@tcgilbert We have checked our logs and haven't found any recent activity for calls from Segment containing mailingLists data (anything with that key have an empty array). We are happy to make this change ASAP so it can roll out. We'd really like to avoid creating a new V2 action

Hey @danrowden - @joe-ayoub-segment and I discussed, and we think it will fine to simply overwrite the existing field. If you have tested my recommendation and it works for you I can re-open this PR.

Otherwise, feel free to take this as a reference and submit your own PR that updates the maillingLists field

tcgilbert avatar Sep 27 '24 14:09 tcgilbert

@tcgilbert I tested sending data in traits, both mapping them in the UI and without mapping and found some things to fix, which I've done in this PR: https://github.com/segmentio/action-destinations/pull/2464

danrowden avatar Sep 27 '24 15:09 danrowden

thanks @danrowden. I see what you mean about the mailingLists in traits now. I've added my review here and will close this one out.

tcgilbert avatar Sep 27 '24 17:09 tcgilbert