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

User v2 #334

Merged
merged 18 commits into from
Oct 31, 2018
Merged

User v2 #334

merged 18 commits into from
Oct 31, 2018

Conversation

carycheng
Copy link

No description provided.

@boxcla
Copy link

boxcla commented Aug 22, 2018

Hi @carycheng, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla

Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks!

@mattwiller mattwiller changed the base branch from 1.5 to master August 24, 2018 17:16
@coveralls
Copy link

coveralls commented Oct 19, 2018

Pull Request Test Coverage Report for Build 1287

  • 52 of 52 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 97.026%

Totals Coverage Status
Change from base Build 1269: 0.06%
Covered Lines: 2447
Relevant Lines: 2522

💛 - Coveralls

"""
url = self.get_url('email_aliases', email_alias.object_id)
response = self._session.delete(url, expect_json_response=False)
return response.ok
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be return email_alias.delete()

Is there any reason this method needs to exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

The URL this hits is /users/USER_ID/email_aliases/EMAIL_ALIAS_ID; I wasn't sure if there was a good pattern for encapsulating both in a single object. Is there a better way to handle this sort of case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't notice that. I guess this is fine for now then.

Or we can move this implementation into an override of delete() on EmailAlias, and have it enforce that you give a user input.

'notify': notify,
'force': force,
}
response = self._session.delete(url, params=params, expect_json_response=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be return super(User, self).delete(params=params)

@mattwiller mattwiller merged commit 4b7a8fe into master Oct 31, 2018
@mattwiller mattwiller deleted the user_v2 branch October 31, 2018 23:20
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.

None yet

5 participants