ropensci / software-review

rOpenSci Software Peer Review.
292 stars 104 forks source link

submission: qualtRics #192

Closed JasperHG90 closed 6 years ago

JasperHG90 commented 6 years ago

Summary

This package focuses on the retrieval of survey data using the Qualtrics API and aims to reduce the pre-processing steps needed to prepare this data for analysis. It also supports the import of manual data exports and other metadata (survey flow, questions, number of responses etc.).

Package: qualtRics
Type: Package
Title: Download Qualtrics Survey Data Directly into R
Version: 3.0
Author: Jasper Ginn
Maintainer: Jasper Ginn <jasperginn@gmail.com>
Description: Qualtrics <https://www.qualtrics.com/about/> 
    allows users to collect online data through surveys.
    This package contains convenience functions to pull
    survey results straight into R using the Qualtrics
    API. See <https://api.qualtrics.com/> for more 
    information about the Qualtrics API. This package is 
    community-maintained and is not officially supported 
    by Qualtrics.
Imports:
    httr,
    stringr,
    readr,
    jsonlite,
    assertthat,
    sjlabelled,
    lubridate,
    yaml,
    dplyr,
    rlang
URL:
    https://jasperhg90.github.io/qualtRics/
BugReports:
    https://github.com/JasperHG90/qualtRics/issues
License: GPL-3 | file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.0.1
Suggests: 
    knitr,
    rmarkdown,
    httptest
VignetteBuilder: knitr

https://github.com/JasperHG90/qualtRics

I would categorise it under 'data retrieval', since the package allows users to retrieve data exports from Qualtrics and import this data directly into R.

Target audience are analysts who use Qualtrics to design and disseminate surveys and who want straightforward access to their data using the API.

Not entirely. There are several packages that provide functionality to interact with the Qualtrics API, e.g.

However, I'm not sure if these packages are still under active development. qualtRics focuses entirely on importing survey data and attempts to make this as simple as possible. Furthermore, it is the only package that uses survey metadata to automatically convert survey questions.

No pre-submission, but I was contacted by one of your colleagues who invited me to submit the package. See here

Requirements

Confirm each of the following by checking the box. This package:

Publication options

Already on CRAN. See CRAN page

Detail

No warnings / errors

Yes with following notes:

  1. Function names: these do not conform to the rOpenSci standards. I chose to keep them as is since this package has already been on CRAN for some time and changing the names of the functions would result in useless existing code for users.

  2. News file: Until recently I used a Changelog.md file for news about changes. I'm now using rOpenSci headers for new releases, but have not done so for previous releases.

karthik commented 6 years ago

Editor checks:


Editor comments

We understand your desire to keep function names the same to avoid breaking code for current users. The exception is noted.

Thanks for switching to the rOpenSci headers. Below are the comments from goodpractice. Please address the relevant comments as I work towards finding reviewers. You can run goodpractice::gp() on your package anytime as you fix these issues.

── GP qualtRics ────────────────────────────────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code
    in general. 55% of code lines are covered by test cases.

    R/getSurveyQuestions.R:52:NA
    R/getSurveyQuestions.R:54:NA
    R/getSurveyQuestions.R:56:NA
    R/getSurveyQuestions.R:57:NA
    R/getSurveyQuestions.R:58:NA
    ... and 262 more lines

  ✖ use '<-' for assignment instead of '='. '<-' is the
    standard, and R users and developers are used it and it is easier
    to read your code for them if you use '<-'.

    R/utils.R:106:11

  ✖ avoid long code lines, it is bad for readability. Also,
    many people prefer editor windows that are about 80 characters
    wide. Try make your lines shorter than 80 characters

    R/assertions.R:21:1
    R/assertions.R:27:1
    R/assertions.R:29:1
    R/assertions.R:30:1
    R/assertions.R:35:1
    ... and 83 more lines

  ✖ avoid calling setwd(), it changes the global environment.
    If you need it, consider using on.exit() to restore the working
    directory.

    tests/testthat/test-getSurvey_reads_stored_survey.R:7:3
    tests/testthat/test-registerOptions_reads_from_file.R:15:3

  ✖ avoid sapply(), it is not type safe. It might return a
    vector, or a list, depending on the input data. Consider using
    vapply() instead.

    R/getSurveyQuestions.R:54:20
    R/getSurveyQuestions.R:57:32
    R/getSurveyQuestions.R:58:34
    R/getSurveyQuestions.R:59:36
    R/getSurveyQuestions.R:65:21
    ... and 1 more lines

  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...),
    1:NROW(...) and 1:NCOL(...) expressions. They are error prone and
    result 1:0 if the expression on the right hand side is zero. Use
    seq_len() or seq_along() instead.

    R/getSurveyQuestions.R:69:23
    R/utils.R:464:56

  ✖ fix this R CMD check NOTE: Namespace in Imports field not
    imported from: ‘lubridate’ All declared Imports should be used.
  ✖ fix this R CMD check WARNING: LaTeX errors when creating
    PDF version. This typically indicates Rd problems.
  ✖ fix this R CMD check ERROR: Re-running with no
    redirection of stdout/stderr. Hmm ... looks like a package You may
    want to clean up by 'rm -rf /tmp/RtmpTvxGKw/Rd2pdf7cb12ddc5dd3'
────────────────────────────────────────────────────────────────────────────────

Reviewer 1: @kierisi Due date: Feb 25

Reviewer 2: @trinker Due date: March 7

JasperHG90 commented 6 years ago

Hi Karthik,

Thanks for your comments.

I fixed the following gp feedback:

Partially fixed or left as is:

screen shot 2018-02-05 at 10 48 49

I've added the latest gp output below.


GP qualtRics

It is good practice to

✖ write unit tests for all functions, and all package code in general. 56% of code lines are covered by test cases.

R/getSurvey.R:127:NA
R/getSurvey.R:128:NA
R/getSurvey.R:129:NA
R/getSurvey.R:130:NA
R/getSurvey.R:131:NA
... and 258 more lines

✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80 characters wide. Try make your lines shorter than 80 characters

inst/doc/qualtRics.R:8:1
inst/doc/qualtRics.R:11:1
inst/doc/qualtRics.R:14:1
R/assertions.R:21:1
R/assertions.R:27:1
... and 85 more lines

✖ avoid calling setwd(), it changes the global environment. If you need it, consider using on.exit() to restore the working directory.

tests/testthat/test-getSurvey_reads_stored_survey.R:8:3
tests/testthat/test-getSurvey_reads_stored_survey.R:9:11
tests/testthat/test-registerOptions_reads_from_file.R:17:3
tests/testthat/test-registerOptions_reads_from_file.R:18:11

✖ fix this R CMD check WARNING: LaTeX errors when creating PDF version. This typically indicates Rd problems. LaTeX errors found: ! LaTeX Error: File `inconsolata.sty' not found. Type X to quit or to proceed, or enter new name. (Default extension: sty) ! Emergency stop. <read *> l.276 ^^M ! ==> Fatal error occurred, no output PDF file produced!


maelle commented 6 years ago

Just a note regarding long lines that can only be long lines like error messages, you can add # nolint at the end of those lines https://github.com/jimhester/lintr#project-configuration this way lintr will not see them. Not te be abused of of course 👼

JasperHG90 commented 6 years ago

Reviewed all lines and added # nolint where appropriate

karthik commented 6 years ago

Reviewer 2 is @trinker (due date: by March 7th)

kierisi commented 6 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [ ] A short summary describing the high-level functionality of the software
  • [ ] Authors: A list of authors with their affiliations
  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 3


Review Comments

This package allows an R user to access Qualtrics survey data quickly and easily within R by using the Qualtrics API feature. As an avid Qualtrics user, I was pleasantly surprised with how quickly and seamlessly I was able to access survey data for analysis, because this is normally a tedious and time-consuming task. I tested the package using surveys of various:

Overall I encountered no difficulty in installing and using the package, even when testing complex surveys with high response rates.

As indicated above, a statement of need has not yet been provided by the package authors. From my perspective, this package addresses several major needs often encountered by analysts who use R to work with Qualtrics data. Speaking broadly, Qualtrics itself can be quite finnicky in how it handles survey response data outside of Qualtrics. The qualtRics package helps mitigate this through the following methods:

Tests

Using devtools::test() had three failures, all with the following error message:

test-getSurveys.R:4: error: (unknown) could not find function "with_mock_api"

Documentation/vignette

The explanation on registering Qualtrics credentials and creating a configuration file are aces. Providing links on where to find the Qualtrics API token that lead to Qualtrics help documentation reinforces what Qualtrics should be providing support on vs. what the package authors can provide support on.

Help documentation and the vignette both provide good baseline information on the package and provide examples for the use of each function. I do recommend adding in line spacing to help improve the readability of the help documentation examples.

trinker commented 6 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 4.5 hrs


Review Comments

Thanks for providing this interface to the R community. I was able to get up and running pretty quickly which is kudos considering I had never used Qualtrics and that the package is an API interface which can be tricky to have a smooth user interface. I have broken down additional comments into sub headings.

Functionality

The package was pretty intuitive and easy to use. There were a few cognitive sticking points as I worked through the Vignette, README, and Examples that are in the form of suggestions below:

  1. Suggest a function for making a '.qualtRics.yml' in the working directory would be nice and add to the user experience. This may negate the need for images showing how to manually make this file.

Messages

Messages (messages, warnings, and errors) were detailed and had suggestions for what to do.

  1. Suggested use of line breaks (i.e., '\n') for qualtRicsConfigFile's message. It currently is 419 characters wide, making it difficult to read. Same is true of the error message in getSurveys for bad url. This is true across many messages, warnings, and errors.

README/Vignette

Between the README and Vignette I was up and running quickly. The links to the Qualtrics page for the base_url and institution specific url were very helpful. I did find the following points that were a little challenging in getting set up:

  1. The README doesn't state "Why is this package needed?" The README points to related work but doesn't compare or show the advantages of qualtRics. The advantage of this package is mentioned in 'paper.md' and in the ROpenSci submission but the later won't be accessible to users.
  2. Numbering process steps as described in the README (e.g., under the Setting up a config file heading) and the Vignette would help the user know what to do an when. A video may be a nice addition as this package requires set up beyond writing code but the screen shots do a good job providing a visual view of the text describing the process.
  3. When using the Vignette I noticed: the term root_url vs. Qualtrics' term base_url could be confusing. It's nice to be consistent when possible. In this case base_url seems to match what Qualtrics calls hostname (The /API doesn't seem to be required by qualtRics).
  4. When using the Vignette I noticed: the root_url parameter for registerOptions or the '.qualtRics.yml' is not consistent with the Qualtrics description which ends the url with /API, using this format described by Qualtrics in the package causes an error. Perhaps (a) automatically stripping /API off in functions like getSurveys or (b) better description of this in the documentation would assist the user.

Examples

Examples ran as expected with correct parameters and tweaks (examples with a package like this are difficult to do out of the box because every user's experience will be different). Nice job with making as generic as possible. I did note the following difficulties and/or suggestions:

  1. readSurvey example has readSurvey("<YOUR-PATH-TO-CSV-FILE>"). A minimal .csv file could easily be included in the inst file and accessed via system.file. This would make the examples more reproducible.
  2. metadata examples uses a slightly different format (and more difficult to follow) for giving surveyId (it uses id which is a variable that doesn't exist) while other function examples use surveyId = surveys$id[6] (or simular) which connects to the prior step in the example and easy to follow.

CRAN Specifications

This appeared to pass CRAN checks. It does violate the following requirements in the Writing R Ext.:

  1. Function titles are lower cased in .Rd files (and roxygen). CRAN Ext. states "Title information for the Rd file. This should be capitalized and not end in a period..."

ROpenSci Packaging Guidelines

The package generally complies with ROpenSci's Packaging Guidelines (https://github.com/ropensci/onboarding/blob/master/packaging_guide.md). Code contains many comments for ease of understanding. There is little if any code duplication. I did note:

  1. Snake case is not used
  2. Mix of snake and camel case in the arguments to getSurvey
  3. Inconsistent function naming convention where some lead with a verb while others do not. Consistency here will allow users to recall function names easier. That being said the package is already on CRAN, slow deprecation required.
  4. The ROpenSci Guidelines suggest Hadely's code styling: http://r-pkgs.had.co.nz/r.html. The Spacing under http://r-pkgs.had.co.nz/r.html#style is violated in some examples such as ?getSurvey we see api_token="<YOUR-API-TOKEN>", root_url="<YOUR-ROOT-URL>") and verbose=TRUE
  5. Code in some .R files could benefit from vertical spacing as well to make it more readable. For example, in 'R/assertions.R' file we see that at lines 55-56 there are 2 functions that have no space between. Often multiple nested functions are on one or two lines. Breaking up these lines and adding indenting would improve readability.

Tests

Testing things that are secret (like API keys) is difficult. This package runs as many tests as possible. I did note:

  1. devtools::test had three failures: could not find function "with_mock_api" If I load library(httptest) it works. This may just be ignorance on my part on usage. Travis-CI seems to know how to handle this so it's likely me.
JasperHG90 commented 6 years ago

Thanks for the reviews! I expect to start incorporating your feedback at the end of the week, as I'm currently wrapping up a project for a client.

karthik commented 6 years ago

Thanks @JasperHG90! Looking forward to your revisions.

JasperHG90 commented 6 years ago

Started working on revisions. Expect to finish by Friday 23 March at latest.

JasperHG90 commented 6 years ago

Hello all,

Thank you for your reviews. They were very useful. I've addressed most of them in the code (see the development branch), and have placed several comments/questions at the bottom of this post.

Updates

Questions/comments

Function titles are lower cased in .Rd files (and roxygen). CRAN Ext. states "Title information for the Rd file. This should be capitalized and not end in a period..."

Could you provide a link/example?

trinker commented 6 years ago

@JasperHG90 Sorry for the lack of clarity on the lower cased titles:

CRAN policies:: I don't understand this comment:

What I mean is that cran states:

Function titles are lower cased in .Rd files (and roxygen). CRAN Ext. states "Title information for the Rd file. This should be capitalized and not end in a period..."

If you go to one of your files: https://github.com/JasperHG90/qualtRics/blob/master/R/getSurvey.R

We see:

' Export a survey and download into R

at line 17. This should not be lower cased but in title case:

' Export a Survey and Download into R

This will then transfer to the .Rd files when you roxygenize.

My thinking on the failed test is that it only fails for devtools::test and since it works on Travis-CI and when I run a CRAN check I'd say this can be ignored. I wouldn't move httptest to Imports unless you're actually importing it. I'd appreciate others to weigh in here.

JasperHG90 commented 6 years ago

Thanks for your comments @trinker . I edited the titles.

karthik commented 6 years ago

@kierisi Hi Jesse! 👋 Could you please take a look at @JasperHG90's recent changes and see if you are able to check off a statement of need issue? And please let us know if the fixes and updates are ok by you for a sign off. 🙏

kierisi commented 6 years ago

clicked and on-board for a sign off :rocket:

karthik commented 6 years ago

@trinker Do you have other concerns that remain unaddressed or are you happy to sign off as well?

trinker commented 6 years ago

@karthik happy to sign off as well

karthik commented 6 years ago

Approved! 🎉 Thank you for submitting and @kierisi and @trinker for thorough and timely reviews! 🙏

@JasperHG90

To-dos:

[![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). If you are, @stefaniebutland will be in touch about content and timing.

JasperHG90 commented 6 years ago

To-dos:

JasperHG90 commented 6 years ago

Happy to write a technical note!

stefaniebutland commented 6 years ago

@JasperHG90 Great to hear that you'd like to publish a technote on this. We can publish at any time (in contrast to blog posts that are scheduled). Please submit a draft by pull request at your convenience and we can review before publishing.

Here are examples: https://ropensci.org/technotes/

Instructions on pull request and preview: https://github.com/ropensci/roweb2#contributing-a-blog-post. The only difference in YAML for technote is categories:

I will add the topicid before publishing.

Don't hesitate to ask any questions here.

JasperHG90 commented 6 years ago

Hi Stefanie. Thanks for the information!

karthik commented 6 years ago

@JasperHG90 One quick question. Would you be interested in adding @kierisi and @trinker as reviewers in your description file? Read more about why here

Also would be good to see if they're both comfortable with the idea.

e.g. https://github.com/ropensci/rdflib/blob/master/DESCRIPTION#L8-L10

kierisi commented 6 years ago

fine by me!

JasperHG90 commented 6 years ago

What a neat feature! If @Kierisi and @trinker are OK, i’d Like to add them as reviewers. Please leave information in a comment so I can add!

On Wed, 11 Apr 2018 at 19:31, Karthik Ram notifications@github.com wrote:

@JasperHG90 https://github.com/JasperHG90 One quick question. Would you be interested in adding @kierisi https://github.com/kierisi and @trinker https://github.com/trinker are reviewers in your description file? Read more about why here https://ropensci.org/blog/2018/03/16/thanking-reviewers-in-metadata/

Also would be good to see if they're both comfortable with the idea.

e.g. https://github.com/ropensci/rdflib/blob/master/DESCRIPTION#L9-L11

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/192#issuecomment-380533412, or mute the thread https://github.com/notifications/unsubscribe-auth/AHtJuw8OnBE_Trr4GLu2q0Au5dlyneVzks5tnj3TgaJpZM4R4hH5 .

karthik commented 6 years ago

@kierisi @trinker If you don't already have an ORCID, sign up for one here: https://orcid.org/

kierisi commented 6 years ago

here you go: 0000-0003-1915-9612

trinker commented 6 years ago

0000-0002-4299-7692

JasperHG90 commented 6 years ago

@karthik had one more question: could you explain to me how the JOSS process works? Is the accompanying paper automatically submitted or do I need to do something? Thanks!

karthik commented 6 years ago

@JasperHG90 Sure. Just follow the submission instructions for JOSS (http://joss.theoj.org/papers/new) and submit a paper there. You'll need to write the paper of course, and include some high level references in a .bib file, then commit both to this same repo.

Then tag me as editor. I'll do a quick review of the paper itself and make sure it reads well. Once that's cleared, you will have to archive a copy of your repo in Zenodo (zenodo.org) and share the DOI. After that the EIC and I can finish the final steps for acceptance.

karthik commented 6 years ago

@JasperHG90 I'm going to close this issue here but feel free to ping me over email (see my profile) for help with the JOSS part. 🙏

karthik commented 6 years ago

Paper is now published on JOSS! 🎉 http://joss.theoj.org/papers/10.21105/joss.00690

stefaniebutland commented 6 years ago

@JasperHG90 Are you still interested/ willing to write a tech note about qualtRics? I think it would really be of interest and would be great to bring some attention to the package

JasperHG90 commented 6 years ago

Hi Stefanie,

Thanks for your message. I'd be happy to do this but I am currently involved in a couple of projects that are taking up all of my time. Things should ease up around half August. I'll put this on the to-do list for around that time!

Best,

Jasper.

stefaniebutland commented 6 years ago

I'll ping you here in late August then @JasperHG90 so we can come up with a tentative date for a draft. Cheers

stefaniebutland commented 6 years ago

@JasperHG90 Hoping your projects load allows you some breathing room soon. I have blog post publication dates available Sept 18, Oct 2, 9, 16. If you're still interested, let me know your preferred date and mark your calendar to submit a draft a week in advance. Goal is to get more eyes on your work, but I know this is extra work beyond the review process. No worries if you're reluctant to spend the extra time.

stefaniebutland commented 6 years ago

Hi @JasperHG90. One last ping to ask if you want to write a qualtRics post.