Skip to content

Allow exact group matches #261

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

Merged
merged 19 commits into from
Oct 30, 2024
Merged

Allow exact group matches #261

merged 19 commits into from
Oct 30, 2024

Conversation

jonkeane
Copy link
Contributor

Adds an exact parameter to groups_create_remote to allow creating a specific group even if there are multiple matches. This follows a similar change in #157

This as well as the matching users_create_remote could use some refactoring, though I'm not sure it's worth it in this PR.

Resolves #216

test_that("groups_create_remote creates multiples if multiple are found", {
expect_error(
groups_create_remote(con, "Art"),
"The expected group\\(s\\) were not found"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how this matches the test description

@anders-wiggers
Copy link

Hi,
What is the status of this PR? Is it stuck? Is there anything we can assist with to complete the feature? This is required for our enterprise solution.

@toph-allen toph-allen self-assigned this Sep 13, 2024
toph-allen and others added 4 commits September 13, 2024 15:59
# Conflicts:
#	tests/testthat/2024.08.0/__api__/v1/groups-2c8d16-PUT.json
#	tests/testthat/2024.08.0/__api__/v1/groups-8a8b3e-PUT.json
#	tests/testthat/2024.08.0/__api__/v1/groups-8e9a47.json
#	tests/testthat/2024.08.0/__api__/v1/groups-c01bad.json
#	tests/testthat/2024.08.0/__api__/v1/groups/remote-519747.json
#	tests/testthat/2024.08.0/__api__/v1/groups/remote-d27795.json
Comment on lines 13 to 18
test_that("groups_create_remote works if match already exists", {
expect_message(
groups_create_remote(con, "Everyone"),
"Creating remote group"
)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to test the check == FALSE behavior?

test_that("groups_create_remote creates match if one is found", {
skip("not implemented")
})
test_that("groups_create_remote creates multiples if multiple are found", {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test wording seems to suggest that it's intended to test the behavior when expect > 1. But there's also the skeleton of a test for an error if expect > 1.

@toph-allen
Copy link
Collaborator

toph-allen commented Oct 25, 2024

I made the following changes, and could use feedback on a few things:

Testing. Some tests were implemented using the MockConnect class. I was able to use some of the existing JSON files to get existing tests to work. There are tradeoffs to each workflow. Right now, upsides of specifying mocks in-line are (1) I find it easier to understand what information is returned by any given call, (2) the ability to have multiple different pieces of data returned from a given URL, (3) verifying the routes that were called. The main functional downside is that it lacks verification of the expected request body or query string, which I'd like to address. The more verbose tests are perhaps a downside too; there's definitely a tradeoff between that and having easily-discoverable context. I can leave it as-is or convert all the tests to a single type. (I would find this easier to go in the direction of mock_dir to in-line.)

Return type. The functions previously returned a single user or group, which (1) was not guaranteed to even be the created group (since search happens by prefix there's no guarantee that the returned group was gonna be the one created) and (2) was nonsensical in the case of creating multiple groups in a single request. I changed this to return the aggregated output of the group/user creation API calls. However, because these could potentially be multiple calls, this is currently a list of lists. This is also technically a breaking change. 🤔

Three possible solutions I can think of:

  1. Keep the change as is. Address in a future PR where groups are actually given a class.
  2. If we're making a breaking change, break it even more! Have the functions just return a data frame with each line representing a created user or group. That's at least nicer to look at and work with, and is probably what R users expect.
  3. Cap the value of expect at 1. This would prevent the creation of multiple users or groups with a single function call and allow us to revert to the old return type. There was a not-implemented test that suggested this might have been someone's intent at some point, but the stated expectation of the test and the behavior of the code differed.
  4. Revert to the old return type (a single group) but at least make it function correctly in the case of a single group created.

[edit] @jonkeane I can't flag you for review on your own PR apparently, but comments would still be appreciated. :)

toph-allen and others added 2 commits October 28, 2024 15:08
Co-authored-by: Barret Schloerke <[email protected]>
Co-authored-by: Barret Schloerke <[email protected]>
@toph-allen
Copy link
Collaborator

@schloerke Thanks for your suggestions! Do you have any thoughts on my brain-dump in this comment?

@toph-allen toph-allen requested a review from schloerke October 28, 2024 19:09
@toph-allen toph-allen self-requested a review October 29, 2024 17:37
@toph-allen
Copy link
Collaborator

Reverted the return value after investigating get_groups with @schloerke. Updated incorrect docs for get_groups as a result. :)

Copy link
Collaborator

@toph-allen toph-allen left a comment

Choose a reason for hiding this comment

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

Accidentally re-requested review from myself; getting it off my list.

@toph-allen toph-allen merged commit cd8291a into main Oct 30, 2024
19 checks passed
@toph-allen toph-allen deleted the groups branch October 30, 2024 20:40
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.

Trying to create groups but search is returning two simular group names
6 participants