Skip to content

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
@botder
Copy link
Member

botder commented Jul 9, 2018

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

@ccw808
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'

@Dezash
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.

@Dezash
Copy link
Contributor Author

Dezash commented Jul 9, 2018

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.

@ccw808
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

@Dezash
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?

@CrosRoad95
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.

@Dezash
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.

@Citizen01
Copy link
Member

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

@Dezash
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.

@Dezash
Copy link
Contributor Author

Dezash commented Jul 9, 2018

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???

@CrosRoad95
Copy link

Implement findAccounts with regex support :)

@ccw808
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

@ccw808
Copy link
Member

ccw808 commented Jul 9, 2018

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

@Dezash
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.

@qaisjp
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
@Dezash
Copy link
Contributor Author

Dezash commented Jul 22, 2018

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

@qaisjp
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?

@Dezash
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
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants