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

The user.sunet_id is saved w/ the email not the username #315

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

SaravShah
Copy link
Contributor

@SaravShah SaravShah commented Sep 19, 2018

RSpec tests use only the username, need to fix to use the email

Connects to #309

RSpec tests use only the username, need to fix to use the email
@coveralls
Copy link

Pull Request Test Coverage Report for Build 962

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 58.513%

Totals Coverage Status
Change from base Build 952: 0.0%
Covered Lines: 543
Relevant Lines: 928

💛 - Coveralls

@atz
Copy link
Contributor

atz commented Sep 19, 2018

I'm confused on this one. I thought the sunet_id was what was part of the model, not the email. But this treats the field as containing the full address.

@SaravShah
Copy link
Contributor Author

SaravShah commented Sep 19, 2018

Ah the sunet_id is already in email form:

 #<User id: 1, sunet_id: "saravs@stanford.edu", created_at: "2018-09-17 19:55:49", updated_at: "2018-09-17 19:55:49">

just trying to make the tests show how it is in stage and prod.

we can do #309 before this..... up to ya'll

Copy link
Contributor

@ndushay ndushay left a comment

Choose a reason for hiding this comment

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

LGTM; is intent to do rename of "sunet_id" field in User (from #315) as a separate thing? Because what we have now is email, not "sunet_id".

@SaravShah
Copy link
Contributor Author

yeah @ndushay im not sure which way we should go, either

  1. keep the field as sunet_id and just add a helper method to strip off everything after the "@" sign
  2. change field to email and leave it as is.

@peetucket
Copy link
Member

I added an .email convenience method on the user model in #302 that we can alter later when/if we split sunet_id from email. I would suggest keeping them separate (i.e. doing a stripping if needed on stroage in the sunet_id column -- would make it future proof if auth handoff changes in the future.

@jmartin-sul jmartin-sul merged commit 5bf6de3 into master Sep 19, 2018
@jmartin-sul jmartin-sul deleted the sunet-email branch September 19, 2018 21:11
@jmartin-sul
Copy link
Member

@atz, yeah, what we get as "sunetid" from webauth/shib is actually the whole email address. my guess is that's something to do w/ shib allowing multiple institutions, and so maybe the domain qualifier really is part of the name? anyway, the tests do now better reflect reality, even if our field name now seems a little confusing.

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.

6 participants