-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add list_directories, list_mailing_lists, and mailing_list_contacts #275
base: main
Are you sure you want to change the base?
Conversation
Thanks for this. @juliasilge, I will say that this does bring back up the conversation in #264 about whether we want to try and get/keep a standardized verb framing for function names. That said, I don't know the mailing list endpoints that well, so @dsen6644 I also think it would be great to have your input on what might work/not make sense tools one would need when working with mailing lists. |
All new functions fall into the "fetch" or "get" category. I decided to use the name of the endpoint in the api documentation for clarity and simplicity, but am completely open to changes to the function names. |
I think in this situation, using the endpoint names makes the most most sense. @dsen6644 have you gotten anybody who has tried this out yet? |
I have not, @jmobrien and @chrisumphlett could you test these functions and see if they work with your environment? You should be able to
|
I don't mind testing. I'm not sure if these objects will exist however... I think a mailing list is created in order to do an email distribution right? If so, we'll have that. I'm not sure about directories |
I used |
I'm not seeing the new functions either. |
Apologies! I didn't notice that the new functions weren't exported and documented yet. If you give it a try now via |
It's working for me. I see two potential problems with the data frame returned by
|
To look at this I'd need to set up some fake mailing lists for testing as I don't have any I can touch right now. I'll get to that a bit later. (Does anyone else have a test list on hand? If so, feel free to drop it here) |
Thanks again @dsen6644! Regarding the date/time issue that @chrisumphlett raised, the issue appears to be these lastModifiedDate = purrr::map_chr(elements, "lastModifiedDate", .default = NA_character_),
creationDate = purrr::map_chr(elements, "creationDate", .default = NA_character_), We usually convert these to POSIXct--but I don't think there's a (That said, IIRC, the nested
|
we could use As for the contact count, it looks like we have to pass a query parameter to the get api request for it to return the actual value... i.e @jmobrien what would be the appropriate way to include that in the api request? Right now we have |
add |
I think that's probably fine, so long as @juliasilge agrees. If it were something to add, something like this would be right as @chrisumphlett mentioned:
except that |
Good to know, it would be important to address if we wanted embedded data associated with our contacts in our mailing lists since we have to pass a similar query |
Added a The more I think about it, maybe keeping Given that, a sense of how many people are on any given list might be useful to users even if it sometimes is imperfect (well, unless it's WAY off, is that likely?). Think that would be important to note in the function documentation, though. |
It's weird that Qualtrics says it's not exact, without a reason. I would
guess that it's probably very close until we see evidence other wise. I
have a lot of lists that I could check it against to validate that
assumption
…On Wed, Sep 7, 2022 at 4:54 PM jmobrien ***@***.***> wrote:
Added a query arg that should help.
The more I think about it, maybe keeping contactCount would be good? Like
all_surveys() the main purpose of 'list_mailing_lists()is likely getting
IDs for use in other functions. But it's also an at-a-glance summary
reference of one's lists--I've certainly usedall_surveys()` that way
sometimes.
Given that, a sense of how many people are on any given list might be
useful to users even if it sometimes is imperfect (well, unless it's WAY
off, is that likely?). Think that would be important to note in the
function documentation, though.
—
Reply to this email directly, view it on GitHub
<#275 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABXAL4T2NHXXUGIRSL2BNOLV5D6JPANCNFSM562RIW4A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Let's run some checks to see how accurate the counts are. |
At first pass these new functions work well for me. I checked the contact counts against my mailing lists in Qualtrics and they match up exactly. One other thing I noticed, |
I'm seeing performance issues as well... @jmobrien wondering if adding the query parameter has effected anything. |
Hmm, I'll take a look. In the meantime, do the issues with I think you should be able to do that with |
Running Running |
Thanks for this. Looks like that's the problem; I'll take a look. |
Finally able to get back to this. Just made a separate PR #301 for adding queries. It uses a slightly different approach that should avoid issues with other types of requests. So, this should free things up to proceed here, with one caveat: commit a6f428d is now not needed and should be removed. @dsen6644, I'm actually not clear how that git procedure is done most appropriately, especially as things center around your fork & branch. Do you or anyone know? |
Hi @juliasilge, @jmobrien and @dsen6644 - just wanted to say thank you to everyone on this thread that helped here, the new functions from #275 are great! Curious if/when they might be merged with the main branch? A colleague of mine ran into issues installing the version with Thanks again, this package is super helpful! |
Thank you so much for this work - I was having data coercion errors with the fetch_mailinglist() function in version 3.1.7. Seemed to be happening no matter which mailing list ID I used. This version has allowed me to download mailing list contacts using mailing_list_contacts() without errors. Takes a little bit of work to get the directoryID but it's working! Thank you and looking forward to when these are incorporated into the main branch. |
I just moved this repo's default branch from |
list_mailing_lists
will replaceall_mailinglists
,mailing_list_contacts
will replacefetch_mailinglist
.Mailing list pulls now require the directory id associated with the mailing list.
Unit testing is still needed (I'll need some directions, haven't used the VCR package in a while), but I wanted to get other developers to test these new functions first.
Should close #271