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

Add promptBotID field for multi linked bot feature #188

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

onevcat
Copy link
Member

@onevcat onevcat commented Jun 27, 2023

This added the promptBotID parameter to login flow. It allows users to append a linked bot ID to the login URL as query when performing login. For now, this is an internal-only feature so we do not contain any doc or description to it.

@onevcat onevcat requested a review from eJamesLin June 27, 2023 07:59
@@ -37,6 +37,7 @@ public class LoginProcess {
let botPrompt: LoginManager.BotPrompt?
let preferredWebPageLanguage: LoginManager.WebPageLanguage?
let onlyWebLogin: Bool
let promptBotID: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

how about making it var and has default value, which could be less modify to the code and interface?
var promptBotID: String? = nil

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I would prefer to follow the current styles, to make the whole configuration properties immutable. If we want to change it to var ... = nil, I will choose to change all other things too. It is a bit big and worth another PR. (I do not oppose it or have a strong preference on this.)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK,currently the LoginManager.Parameters has default nil, and won't impact current users.


let components = URLComponents(url: result, resolvingAgainstBaseURL: false)
let items = components!.queryItems!
XCTAssertEqual(items.count, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

to make the magic number 2 more readable, how about update to the following?

XCTAssertEqual(items.count, ["loginChannelId", "returnUri"].count)

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try to update it later (maybe tomorrow!)

@onevcat
Copy link
Member Author

onevcat commented Jun 28, 2023

@eJamesLin I updated the tests a bit for readability as suggested. Please check again! Thanks.

Copy link
Contributor

@eJamesLin eJamesLin left a comment

Choose a reason for hiding this comment

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

LGTM

@onevcat onevcat merged commit fb17319 into master Jun 28, 2023
13 checks passed
@onevcat onevcat deleted the feature/multi-linked-bot branch June 28, 2023 08:05
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

2 participants