qualtRics icon indicating copy to clipboard operation
qualtRics copied to clipboard

add list_directories, list_mailing_lists, and mailing_list_contacts

Open dsen6644 opened this issue 2 years ago • 29 comments

list_mailing_lists will replace all_mailinglists, mailing_list_contacts will replace fetch_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

dsen6644 avatar Aug 17 '22 18:08 dsen6644

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.

jmobrien avatar Aug 25 '22 21:08 jmobrien

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.

dsen6644 avatar Aug 31 '22 15:08 dsen6644

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?

juliasilge avatar Aug 31 '22 19:08 juliasilge

I have not, @jmobrien and @chrisumphlett could you test these functions and see if they work with your environment?

You should be able to

  • Return a df of directory ids using the list_directories function.
  • Return a df of mailing lists for a directory by passing a directory id to the list_mailing_lists function.
  • Return a df of contacts by passing a mailing list id and its respective directory id to the mailing_list_contacts function.

dsen6644 avatar Sep 01 '22 16:09 dsen6644

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

chrisumphlett avatar Sep 01 '22 18:09 chrisumphlett

I used remotes::install_github("ropensci/qualtRics#275") to install. I'm not seeing these new functions.

chrisumphlett avatar Sep 01 '22 18:09 chrisumphlett

I'm not seeing the new functions either.

pschatz25 avatar Sep 02 '22 19:09 pschatz25

Apologies! I didn't notice that the new functions weren't exported and documented yet. If you give it a try now via remotes::install_github("ropensci/qualtRics#275" it should work now.

juliasilge avatar Sep 04 '22 18:09 juliasilge

It's working for me. I see two potential problems with the data frame returned by list_mailing_lists().

  1. datetime columns are in unix epoch which is not consistent with the rest of the package; and, they are not integers (and the decimals are all zeroes so I do not believe it's intended to be partial seconds)
  2. contactCount column is zero when it shouldn't be.

qual

chrisumphlett avatar Sep 06 '22 13:09 chrisumphlett

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)

jmobrien avatar Sep 07 '22 15:09 jmobrien

Thanks again @dsen6644!

Regarding the date/time issue that @chrisumphlett raised, the issue appears to be these purrr:::map_chr() calls on lines 46/47:

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 map() (or equivalent) for that output. So it might be a bit more than a simple fix.

(That said, IIRC, the nested map() calls were something that were used to handle NULLs or missing elements, yes? You know the raw API output better than I do, @dsen6644--are we likely to encounter these issues here? If not, would one easy option be to just convert & stack the key variables similarly to how it's done in `all_surveys()?)

contactCount should presumably also be numeric (currently character). But even given that, I don't (yet) see in the code why that would be 0, unless that's what the endpoint returned.

jmobrien avatar Sep 07 '22 16:09 jmobrien

we could use lubridate::as_datetime(purrr::map_dbl(...)/1000)) to solve for the UNIX time.

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 list(includeCount = "true").

@jmobrien what would be the appropriate way to include that in the api request? Right now we have res <- qualtrics_api_request("GET", url = fetch_url).... I tried res <- qualtrics_api_request("GET", url = fetch_url, body = list(includeCount = "true") without success. What's the appropriate way to pass query parameters?

dsen6644 avatar Sep 07 '22 18:09 dsen6644

add query = list(param1= "value1", param2= "value2"))

chrisumphlett avatar Sep 07 '22 18:09 chrisumphlett

I think I'm actually going to drop the contact count, since it's not "exact"... which could lead to some confusion. See the documentation.

image

dsen6644 avatar Sep 07 '22 18:09 dsen6644

I think I'm actually going to drop the contact count, since it's not "exact"... which could lead to some confusion. See the documentation.

image

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:

add query = list(param1= "value1", param2= "value2"))

except that qualtrics_api_request() isn't configured to correctly pass that to httr::RETRY() via the .... The ability to add other params might be worthwhile so I'll look into adding that, but I guess it's moot for now.

jmobrien avatar Sep 07 '22 19:09 jmobrien

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 includeEmbedded.

dsen6644 avatar Sep 07 '22 19:09 dsen6644

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 used all_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.

jmobrien avatar Sep 07 '22 20:09 jmobrien

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 https://github.com/ropensci/qualtRics/pull/275#issuecomment-1239868950, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABXAL4T2NHXXUGIRSL2BNOLV5D6JPANCNFSM562RIW4A . You are receiving this because you were mentioned.Message ID: @.***>

chrisumphlett avatar Sep 07 '22 21:09 chrisumphlett

includeCount (list_mailing_lists) and includeEmbedded (mailing_list_contacts) query parameters have been added.

Let's run some checks to see how accurate the counts are.

dsen6644 avatar Sep 08 '22 20:09 dsen6644

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, all_surveys() doesn't seem to run now. It just hangs forever. EDIT: I'm realizing that all_surveys() just doesn't work using this branch, but it still works fine with the published version. My bad.

pschatz25 avatar Sep 09 '22 19:09 pschatz25

I'm seeing performance issues as well... @jmobrien wondering if adding the query parameter has effected anything.

dsen6644 avatar Oct 03 '22 18:10 dsen6644

Hmm, I'll take a look. In the meantime, do the issues with all_surveys() go away (or whatever other performance issues you're having) if you install from the commit just before the new query param? I guess that would be f8bc2ea.

I think you should be able to do that with remotes::install_github("ropensci/qualtRics", ref = "f8bc2ea")

jmobrien avatar Oct 04 '22 01:10 jmobrien

Running all_surveys() withremotes::install_github("ropensci/qualtRics#275") did not execute after ~5 minutes.

Running all_surveys() with remotes::install_github("ropensci/qualtRics", ref = "f8bc2ea") did execute and returned expected results.

pschatz25 avatar Oct 18 '22 21:10 pschatz25

Thanks for this. Looks like that's the problem; I'll take a look.

jmobrien avatar Oct 26 '22 12:10 jmobrien

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?

jmobrien avatar Dec 07 '22 17:12 jmobrien

Let's give @dsen6644 a few days to catch up and respond if possible, but if he is not available, I think the best/easiest thing to do is a new PR. We can make sure to credit @dsen6644 for his work in the new PR and link to this one.

juliasilge avatar Dec 09 '22 15:12 juliasilge