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

Remove redundant edittext_login_id #26

Open
wants to merge 1 commit into
base: grade-projection
Choose a base branch
from
Open

Remove redundant edittext_login_id #26

wants to merge 1 commit into from

Conversation

oja
Copy link
Contributor

@oja oja commented Apr 16, 2017

The value of edittext_login_id can be calculated from the value of edittext_login_username, so it is not needed.

@oja oja changed the title Remove redundant edittext_login_id Remove redundant edittext_login_id Apr 16, 2017
@ehsanmasdar
Copy link
Member

Student id is used to select a student for multi student parent accounts, which could not be parsed in this way

@patil215
Copy link
Member

patil215 commented Apr 17, 2017

Yep, what @ehsanmasdar said is accurate. Unless TEAMS has changed, you can't do what this pull request is suggesting, since there are certain accounts where there are multiple student IDs tied to one username.

If you really want to remove the student ID box, one option is to have an additional spinner style selection prompt pop up if the parser detects multiple student IDs in the account when logging in.

@oja
Copy link
Contributor Author

oja commented Apr 17, 2017

I've seen a couple of students get confused about providing the same information twice on login, so I think its probably best to remove the third box.

In order to cover the case of multistudent accounts @ehsanmasdar brought up: maybe there could be a prompt for that information after clicking the login button (similar to the existing "secondary login" one)? Or do you think the whole thing should be left as-is?

@patil215
Copy link
Member

Yep, the secondary prompt is exactly what I suggested above - I think that'd definitely be a great improvement over the current implementation.

@oja
Copy link
Contributor Author

oja commented Apr 17, 2017

Sorry, didn't see your edit to that comment.

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.

3 participants