Skip to content

Fix: Bug, Add confirmation dialog in onNewIntent / onCreate before any otpauth:// import#305

Open
zishanfiroz wants to merge 6 commits into
helloworld1:masterfrom
zishanfiroz:fix/main-activity-correction
Open

Fix: Bug, Add confirmation dialog in onNewIntent / onCreate before any otpauth:// import#305
zishanfiroz wants to merge 6 commits into
helloworld1:masterfrom
zishanfiroz:fix/main-activity-correction

Conversation

@zishanfiroz

Copy link
Copy Markdown

Added confirmation dialog in onNewIntent / onCreate before any otpauth:// import

@helloworld1

Copy link
Copy Markdown
Owner

Thanks for the contribution. Could you just change the confirmation dialog, not reformatting code to make the PR clear?

@zishanfiroz

Copy link
Copy Markdown
Author

Done as you said, one file also need changes "app/src/main/res/values/strings.xml" which i also did

windowInsets
}

ViewCompat.setOnApplyWindowInsetsListener(binding.addTokenFab) { view, windowInsets ->

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This commit should have already contained this change 40151ec

val rawPath = uri.path?.trimStart('/') ?: ""
val issuer = uri.getQueryParameter("issuer")
?: rawPath.substringBefore(":").ifEmpty { getString(R.string.unknown_issuer) }
val account = rawPath.substringAfter(":", "").ifEmpty { getString(R.string.unknown_account) }

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Empty account should not be allowed. And issuer can be absent but strongly recommended. If issuer is empty, then leave it empty.

Comment thread app/src/main/res/values/strings.xml Outdated
<!-- Confirmation dialog shown when an external app or webpage supplies an otpauth:// URI -->
<string name="add_token_confirmation_title">Add OTP Account?</string>
<string name="add_token_confirmation_message">An external source wants to add an OTP account:\n\nIssuer: %1$s\nAccount: %2$s\n\nOnly proceed if you trust this source.</string>
<string name="unknown_issuer">Unknown</string>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comments before. Empty issuer is OK, empty account is not allowed. Do not need to add those strings

@helloworld1

Copy link
Copy Markdown
Owner

Done as you said, one file also need changes "app/src/main/res/values/strings.xml" which i also did

Thanks for making the changes. I have some comments

@zishanfiroz

Copy link
Copy Markdown
Author

check if its right or i messed up?

@helloworld1

Copy link
Copy Markdown
Owner

Please address the comment I left. Otherwise, I won't be able to merge. Thanks!

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.

2 participants