Skip to content
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dsen6644
Copy link
Contributor

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

@jmobrien
Copy link
Collaborator

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.

@dsen6644
Copy link
Contributor Author

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.

@juliasilge
Copy link
Collaborator

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?

@dsen6644
Copy link
Contributor Author

dsen6644 commented Sep 1, 2022

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.

@chrisumphlett
Copy link
Contributor

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
Copy link
Contributor

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

@pschatz25
Copy link

I'm not seeing the new functions either.

@juliasilge
Copy link
Collaborator

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.

@chrisumphlett
Copy link
Contributor

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

@jmobrien
Copy link
Collaborator

jmobrien commented Sep 7, 2022

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
Copy link
Collaborator

jmobrien commented Sep 7, 2022

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.

@dsen6644
Copy link
Contributor Author

dsen6644 commented Sep 7, 2022

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?

@chrisumphlett
Copy link
Contributor

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

@dsen6644
Copy link
Contributor Author

dsen6644 commented Sep 7, 2022

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

@jmobrien
Copy link
Collaborator

jmobrien commented Sep 7, 2022

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.

@dsen6644
Copy link
Contributor Author

dsen6644 commented Sep 7, 2022

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.

@jmobrien
Copy link
Collaborator

jmobrien commented Sep 7, 2022

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.

@chrisumphlett
Copy link
Contributor

chrisumphlett commented Sep 7, 2022 via email

@dsen6644
Copy link
Contributor Author

dsen6644 commented Sep 8, 2022

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.

@pschatz25
Copy link

pschatz25 commented Sep 9, 2022

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.

@dsen6644
Copy link
Contributor Author

dsen6644 commented Oct 3, 2022

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

@jmobrien
Copy link
Collaborator

jmobrien commented Oct 4, 2022

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")

@pschatz25
Copy link

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.

@jmobrien
Copy link
Collaborator

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

@jmobrien
Copy link
Collaborator

jmobrien commented Dec 7, 2022

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?

@juliasilge
Copy link
Collaborator

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.

@joshuarbruce
Copy link

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 remotes::install_github("ropensci/qualtRics#275") as referenced above by @pschatz25. However, the solution described installs a slightly different version of the mailing_list_contacts() command; it lacks the internal function that pulls embedded data from the referenced mailing list. Installing the package via remotes::install_github("ropensci/qualtRics#275") includes the embedded data function.

Thanks again, this package is super helpful!

@benhmin
Copy link

benhmin commented Jan 5, 2023

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.
Error: Can't coerce element 1 from a character to a logical

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.

@juliasilge
Copy link
Collaborator

I just moved this repo's default branch from master to main; you can use usethis::git_default_branch_rediscover() to update your local setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in qualtrics_response_codes() for fetch_distributions() & 0 observations for all_mailinglists()
7 participants