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

Fix how multiple pin name matches from a Connect board are filtered #835

Merged
merged 3 commits into from
Aug 2, 2024

Conversation

toph-allen
Copy link
Contributor

@toph-allen toph-allen commented Aug 1, 2024

With a Connect board, pins can (and should) be specified with owner/name, as the pin name is unique per user account. This PR fixes a bug that occurs when multiple pins have the same name and the owner is not the first match.

To retrieve content from Connect:

  1. We parse the name into a list with owner and name fields, e.g. "toph/penguins" becomes list(owner = "toph", name = "penguins", full = "toph/penguins)".
  2. We get a list of all content with matching names. For example, we might get a list of two items, the first representing "taylor_steinberg/penguins", the second "toph/penguins".
  3. If there is more than one item, owner must have been specified, so we can filter the list. Because the JSON returned from the API only contains owner GUIDs, we use the Connect API to create a character vector of owner names parallel to the content list, i.e. c("taylor_steinberg", "toph"). We want to use this to filter the list of matching pins.

The old code used json[[name$owner %in% owner_names]]. name$owner %in% owner_names just evaluates to TRUE though, and this would just select the first item in the list. This causes failures like pin_write unexpected permissions errors or unexpectedly updating the wrong pin.

Using json[[which(name$owner == owner_names)]] fixes the problem, as which(name$owner == owner_names) evaluates to the integer index of the matching name.

Tests

I added a test of the new filtering behavior using mocked bindings. Because the main Connect test files are skipped in CI, and this test uses mocks to avoid the need for any network access etc., I put this in a new file with no skip clauses. I'm not sure if there's a better pattern to follow, or if this file should still be skipped on CRAN — let me know if a different approach is better!

Copy link
Member

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @toph-allen! 🙌

@juliasilge juliasilge merged commit 2edcccb into main Aug 2, 2024
14 checks passed
@juliasilge juliasilge deleted the toph/fix-connect-multiple-result-filter branch August 2, 2024 17:24
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants