Skip to content

Update user #300

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 11 commits into from
Jun 25, 2021
Merged

Update user #300

merged 11 commits into from
Jun 25, 2021

Conversation

ceciliastevens
Copy link
Contributor

@ceciliastevens ceciliastevens commented Jun 24, 2021

Description of Change

This change adds a method code42 users update to update a single user, and the corresponding bulk method code42 users bulk update

It partially addresses (but does not fully address) issue #259

Testing Procedure

code42 users update --user-id <user-id> --email <new email>
code42 users bulk update users_to_update.csv

PR Checklist

  • Add unit tests to verify this change
  • Add an entry to CHANGELOG.md describing this change
  • Add docstrings for any new public parameters / methods / classes

@github-actions
Copy link

github-actions bot commented Jun 24, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️

@sdk_options()
def bulk_update(state, csv_rows, format):
"""Update a list of users from the provided CSV."""
csv_rows[0]["updated"] = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this line csv_rows[0]["updated"] = False?

What happens if someone passes in a csv with no rows, does this through some IndexError?
^ if that is the case, and this is needed, we should check the length first and fail early with a better error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to add the column, for some reason if we don't add it beforehand, the addition during handle_row wasn't working properly. I copied this approach from how devices bulk deactivate does it.

The CSV is validated by read_csv(), which will raise an error if the CSV does not contain the proper rows.

@antazoey
Copy link
Contributor

In code42cli.click_ext.groups.py, can you import Py42UsernameMustBeEmailError from py42 (gets raised during update_user()) and add it to the exception list caught in lines 59-69?

def test_update_user_calls_update_user_with_correct_parameters(
runner, cli_state, update_user_success
):
command = ["users", "update", "--user-id", "12345", "--email", "test_email"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you include all the args here so the test confirms they all get passed to the right spot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to test also that it properly passed None where no argument was given (since we don't want the method to accidentally update all unchanged fields to blank values). I've added additional tests to validate that all parameters are passed properly.

…dated" column in devices bulk update; more comprehensive tests
Copy link
Contributor Author

@ceciliastevens ceciliastevens left a comment

Choose a reason for hiding this comment

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

In code42cli.click_ext.groups.py, can you import Py42UsernameMustBeEmailError from py42 (gets raised during update_user()) and add it to the exception list caught in lines 59-69?

I've added this.

@antazoey antazoey self-requested a review June 25, 2021 16:09
Copy link
Contributor

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

Do you mind also changing cmd/devices/.py where it says
csv_rows[0]["deactivated"] = False
to
csv_rows[0]["deactivated"] = "False"

that would be helpful.

This PR looks good!

@ceciliastevens
Copy link
Contributor Author

Do you mind also changing cmd/devices/.py where it says
csv_rows[0]["deactivated"] = False
to
csv_rows[0]["deactivated"] = "False"

that would be helpful.

This PR looks good!

Sure, I'll do that now.

@antazoey antazoey merged commit b2d1f44 into code42:main Jun 25, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 25, 2021
@ceciliastevens ceciliastevens deleted the update-user branch June 25, 2021 16:59
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