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 sunetid used in test to use fully qualified sunet #319

Closed
wants to merge 1 commit into from

Conversation

jmartin-sul
Copy link
Member

No description provided.

@atz
Copy link
Contributor

atz commented Sep 19, 2018

Again, this seems like the wrong direction. We need to keep sunet_id and email as separate ideas. It makes sense to me to use the sunet_id in the directory name to avoid special filesystem characters @ and .. If devise is handing us one thing and we want to use the other, cool, we should:

  • have methods on user that reliably gives us what we want (for email and sunet_id)
  • avoid storing one type of value under an attribute named for the other type

@coveralls
Copy link

Pull Request Test Coverage Report for Build 972

  • 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 59.561%

Totals Coverage Status
Change from base Build 969: 0.0%
Covered Lines: 570
Relevant Lines: 957

💛 - Coveralls

@atz
Copy link
Contributor

atz commented Sep 19, 2018

I don't perceive there as being anything called a "fully qualified sunet id". The sunet ID is the part without the @ sign and domain:
https://cs.stanford.edu/computing-guide/access/sunetid

@jmartin-sul
Copy link
Member Author

yeah, just asked @julianmorley, and he says this is bad naming on UIT's part.

basically, an actual unique shib ID needs a domain name.

sunetid alone is not a valid fully qualified shib ID. so UIT is giving us something in a misleading field name.

i'm not convinced we should key users on sunetid sans domain, though. if we ever have a jsmith@stanford.edu in our system, and a jsmith@lsu.edu, those people will look like the same jsmith, but they aren't.

also, i don't think cs.stanford.edu really provides authoritative docs on UIT practices -- CS department runs a lot of their own servers, and they need to educate their users on broader university conventions, but i wouldn't treat that link as an authoritative doc on what a sunetid is.

uuuuugggghhh

@ndushay
Copy link
Contributor

ndushay commented Sep 19, 2018

d'oh - more failing tests. :-(

@atz
Copy link
Contributor

atz commented Sep 19, 2018

I take your point about shib IDs, but the field we have named is sunet_id, which still doesn't align with treating it like an email address.

We talked about this previously w/ Argo work, iirc, and basically concluded that in the scope for our app would always have a stanford.edu account. Even if we had users w/ other IDs, it was easier to get them (temporary or sponsored) regular sunet_ids than to retool many legacy apps. Would support migrating our "sunet_id" field to "shib_id" or "email". Basically this distinction should be preserved somehow (and I'd prefer the cleaner directory name).

@ndushay
Copy link
Contributor

ndushay commented Sep 19, 2018

"migrating our User model "sunet_id" field to "email" " is mentioned in #309.

@ndushay
Copy link
Contributor

ndushay commented Sep 19, 2018

I'll fix the failing test in the PR I'm working on now.

@ndushay
Copy link
Contributor

ndushay commented Sep 19, 2018

Fixed problem in PR #320 ed06040

Closing this in favor of #320, which has additional commit for issue #78.

@ndushay ndushay closed this Sep 19, 2018
@ndushay ndushay removed the review label Sep 19, 2018
@ndushay ndushay deleted the fix-sunet-id-test branch September 19, 2018 23:03
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

4 participants