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: allow account names with dashed in them to be imported correctly #8370

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

tarekis
Copy link
Contributor

@tarekis tarekis commented Nov 20, 2024

Fixes #8369.

Description of the problem being solved:

Accounts with dashes in them (e.g. Dj-Reck1#9507) could not be imported.
Those accounts are possible because dashes are allowed in account names for consoles.

Steps taken to verify a working solution:

  • Import Dj-Reck1#9507
  • Import Dj-Reck1-9507
  • Import Tarekis#5639
  • Import Tarekis-5639

Hope code looks good to yall. Not a lua dev lol

@@ -1138,6 +1139,24 @@ function HexToChar(x)
return string.char(tonumber(x, 16))
end

function ReplaceCharAtIndex(str, pos, r)
return ("%s%s%s"):format(str:sub(1,pos-1), r, str:sub(pos+1))
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is space-indented; should be tab.

@Regisle
Copy link
Member

Regisle commented Nov 21, 2024

This looks like it works from the few tests I did, but is a little overkill, as the - will always be the 5th character from the end if its used for the discriminator. (discriminator is always the last 5-7 characters)

Also a helper function like ReplaceCharAtIndex should probably go in common.lua incase its actually something we will use in other places.

edit: it works, so unless a maintainer wants the changes you can just leave it as is (and just fix the space indentation), just being nit picky.

@clemenshimmer
Copy link
Contributor

clemenshimmer commented Nov 21, 2024

I totally agree with moving ReplaceCharAtIndex. I overlooked common.lua.

I did the discriminator char index search this over-engineered because it would be forward-compatible to GGG having to adapt to thing like long account names and discriminators being anything other than 4 characters. Since it's not run very often I figured the overkill would be okay for the benefit of also addressing those potential discriminators.

Fixing indent and ReplaceCharAtIndex into common as soon as I'm on a machine I can work with

@tarekis
Copy link
Contributor Author

tarekis commented Nov 21, 2024

Moved ReplaceCharAtIndex and fixed indent.

Copy link
Member

@Wires77 Wires77 left a comment

Choose a reason for hiding this comment

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

Not at a computer I can test this at until later, but looks good to me in general.

Comment on lines 1143 to 1152
reversedAccountName = string.reverse(accountName)
discriminatorIndex = 0
for i = 1, #reversedAccountName do
local c = reversedAccountName:sub(i,i)
if c == "#" or c == "-" then
discriminatorIndex = i
break
end
end
discriminatorIndex = discriminatorIndex * -1
Copy link
Member

@Wires77 Wires77 Nov 21, 2024

Choose a reason for hiding this comment

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

FYI, you don't have to reverse the string and the index here, just put your for loop in reverse instead

I just realized this can all be done in a single gsub

Suggested change
reversedAccountName = string.reverse(accountName)
discriminatorIndex = 0
for i = 1, #reversedAccountName do
local c = reversedAccountName:sub(i,i)
if c == "#" or c == "-" then
discriminatorIndex = i
break
end
end
discriminatorIndex = discriminatorIndex * -1
return accountName:gsub("(.*)[#%-]", "%1#")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know that was possible in lua, cool stuff, but I see you removed it anyways.

gsub not working with regex totally did not click in my brain, props for finding a better shorthand.

@@ -424,7 +424,8 @@ function ImportTabClass:DownloadCharacterList()
accountName = self.controls.accountName.buf:gsub("^[%s?]+", ""):gsub("[%s?]+$", ""):gsub("%s", "+")
end
local sessionID = #self.controls.sessionInput.buf == 32 and self.controls.sessionInput.buf or (main.gameAccounts[accountName] and main.gameAccounts[accountName].sessionID)
launch:DownloadPage(realm.hostName.."character-window/get-characters?accountName="..accountName:gsub("-", "%%23"):gsub("#", "%%23").."&realm="..realm.realmCode, function(response, errMsg)
accountName = ReplaceDiscrimintorSafely(accountName)
Copy link
Member

Choose a reason for hiding this comment

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

Does this behave fine when saving previously imported accounts in settings?

@Wires77 Wires77 merged commit 26a80ef into PathOfBuildingCommunity:dev Nov 22, 2024
1 of 2 checks passed
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.

Dashes in account names are wrongly replaced with discriminator hashes
5 participants