Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add case sensitivity parameter to getAccount #230

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
botder merged 7 commits into multitheftauto:master from Dezash:feature/case-sensitivity
Jan 7, 2019

Conversation

@Dezash
Copy link
Contributor

@Dezash Dezash commented Jul 8, 2018

No description provided.

@Dezash Dezash changed the title (削除) Add case sensitivity parameter (削除ここまで) (追記) Add case sensitivity parameter to getAccount (追記ここまで) Jul 8, 2018
@botder botder added enhancement New feature or request pr:needs-testing labels Jul 9, 2018
Copy link
Member

botder commented Jul 9, 2018

Note for reviewer: Account names are case-sensitive by default, but addAccount supports case-insensitivity.

Copy link
Member

ccw808 commented Jul 9, 2018

If there are two accounts 'bob' and 'BOB', and bob is first in the account database, then getAccount("BOB", nil, true) will return the account for 'bob'

Copy link
Contributor Author

Dezash commented Jul 9, 2018

No it won't.
srun addAccount("bob", "hi", true)
[15:26:23] Console executed command: addAccount("bob", "hi", true)
[15:26:23] Command results: userdata: 075A41F8 [userdata]
srun addAccount("BOB", "hey", true)
[15:26:29] Console executed command: addAccount("BOB", "hey", true)
[15:26:29] Command results: userdata: 075A4400 [userdata]
srun getAccountName(getAccount("BOB", nil, true))
[15:26:54] Console executed command: getAccountName(getAccount("BOB", nil, true))
[15:26:54] Command results: BOB [string]

If you set the third argument to false (case insensitive) then it will return the first match however. Since this is not the default behavior, I think it would be acceptable if a warning in the wiki is added. Returning a table of accounts would make the function much slower so I chose to just return the first result.

Copy link
Contributor Author

Dezash commented Jul 9, 2018
edited
Loading

I just looked into it again and noticed that it generates a an array anyway so it wouldn't make it slower. Should I make it return a table when caseSensitive is set to false?
Edit: Although I'm not so sure why would anyone use this if they had accounts with case variations.

Copy link
Member

ccw808 commented Jul 9, 2018

What about doing a case sensitive search first? If that returns no results then do a case insensitive search

Copy link
Contributor Author

Dezash commented Jul 9, 2018

If there is no case sensitive match, should it return the first case insensitive match or all case insensitive matches?

Copy link

but you can have bob and BOB, there should be option to get N result, by default return table with all found accounts.

Copy link
Contributor Author

Dezash commented Jul 9, 2018

I think returning a table would be inconsistent. Maybe there should be another function for this or an updated getAccounts function that could take a username argument and find all accounts with different case variations.

Copy link
Member

Something like findAccounts could be introduced to prevent braking scripts by changing the returned value type of getAccount

Copy link
Contributor Author

Dezash commented Jul 9, 2018

@Citizen01 it wouldn't break any scripts since no scripts use a third argument yet. What I mean is the function name itself screams one account, singular.

Copy link
Contributor Author

Dezash commented Jul 9, 2018
edited
Loading

What do devs think?

  1. Add username argument to getAccounts
  2. Implement findAccounts
  3. Return a table from getAccount
  4. Leave it as it is (returns exact match and if there is no exact match then the first case-insensitive match)
  5. Something else???

Copy link

Implement findAccounts with regex support :)

Dezash, Citizen01, and AlexTMjugador reacted with laugh emoji

Copy link
Member

ccw808 commented Jul 9, 2018

4. Leave it as it is (returns exact match and if there is no exact match then the first case-insensitive match)

Plus the password should be checked during the search as well

Dezash reacted with thumbs up emoji

Copy link
Member

ccw808 commented Jul 9, 2018

Just to rewind a bit, when would this feature be used by scripters?

Copy link
Contributor Author

Dezash commented Jul 9, 2018

One use would be to make login panels ignore case, thus reducing the chance of people forgetting their credentials.

Copy link
Contributor

qaisjp commented Jul 22, 2018

If 4. Leave it as it is (returns exact match and if there is no exact match then the first case-insensitive match) is accurate, isn't what you've described already possible? As long as you prevent people from creating accounts with the same casing everything should be OK.

@qaisjp qaisjp added the feedback Further information is requested label Jul 22, 2018
Copy link
Contributor Author

Dezash commented Jul 22, 2018

@qaisjp yes, this was to address servers that use case variations for their account usernames.

Copy link
Contributor

qaisjp commented Jul 22, 2018

I see. So you do want to disable the ability to accept case insensitive names because you (e.g) have accounts James and JAMES, and do not want to confuse users with the ability to log in with james?

Copy link
Contributor Author

Dezash commented Jul 22, 2018

No, it is already like that. I want to add an ability to accept case insensitive usernames. If the login panel example is too confusing, consider this: you want to add a real-time username availability checking (as user types their username). If there is an account called "JAMES" and user types in "James", getAccount("James") will return false and the script will say that the username is available even though it isn't since case variation is not allowed by that server. By using the feature from this PR you can check whether any case variation of "james" exists and thus return the correct result.

@patrikjuvonen patrikjuvonen removed the feedback Further information is requested label Sep 5, 2018
@botder botder self-assigned this Jan 2, 2019
Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

Please address @botder's review, otherwise, looks good.

@botder botder merged commit 7401422 into multitheftauto:master Jan 7, 2019
@botder botder added this to the 1.5.7 milestone Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@botder botder botder requested changes

+1 more reviewer

@qaisjp qaisjp qaisjp approved these changes

Reviewers whose approvals may not affect merge requirements

Labels

enhancement New feature or request

Projects

None yet

Milestone

1.5.7

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /