Skip to content

Fixes #6147: Keep Save enabled by default on Authorize to add profiles screen #6193

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.

Sign up for GitHub

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

Open
saimanoharigodavarty wants to merge 4 commits into
base: develop
Choose a base branch
from
+48 −19
Open

Fixes #6147: Keep Save enabled by default on Authorize to add profiles screen #6193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93f6726
Fix part of #6147: keep admin PIN save enabled by default
saimanoharigodavarty Mar 28, 2026
420f502
Fix ktlint error: remove blank line before closing brace
saimanoharigodavarty Apr 2, 2026
a3d44e8
Fix #6147: Disable Save button on PIN validation errors, re-enable on…
saimanoharigodavarty Apr 8, 2026
68ce03e
Fix #6147: Only show confirm mismatch when pin length is valid
saimanoharigodavarty Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions app/src/main/java/org/oppia/android/app/profile/AdminPinActivityPresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ class AdminPinActivityPresenter @Inject constructor(
private val adminViewModel: AdminPinViewModel,
private val resourceHandler: AppLanguageResourceHandler
) {

private var inputtedPin = false
private var inputtedConfirmPin = false

private val args by lazy {
activity.intent.getProtoExtra(
ADMIN_PIN_ACTIVITY_PARAMS_KEY,
Expand Down Expand Up @@ -65,12 +61,9 @@ class AdminPinActivityPresenter @Inject constructor(
adminViewModel.savedPin.get() == it
) {
adminViewModel.savedPin.set(it)
inputtedPin = pin.isNotEmpty()
} else {
adminViewModel.pinErrorMsg.set("")
adminViewModel.savedPin.set(it)
inputtedPin = pin.isNotEmpty()
setValidPin()
}
}
}
Expand All @@ -82,12 +75,9 @@ class AdminPinActivityPresenter @Inject constructor(
adminViewModel.savedConfirmPin.get() == it
) {
adminViewModel.savedConfirmPin.set(it)
inputtedConfirmPin = confirmPin.isNotEmpty()
} else {
adminViewModel.confirmPinErrorMsg.set("")
adminViewModel.savedConfirmPin.set(it)
inputtedConfirmPin = confirmPin.isNotEmpty()
setValidPin()
}
}
}
Expand Down Expand Up @@ -167,7 +157,4 @@ class AdminPinActivityPresenter @Inject constructor(
}
}

private fun setValidPin() {
adminViewModel.isButtonActive.set(inputtedPin && inputtedConfirmPin)
}
Comment on lines -170 to -172
Copy link
Copy Markdown
Member

@BenHenning BenHenning Mar 30, 2026

Choose a reason for hiding this comment

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

Why is this being removed? We do still want to disable the button when there's an error state.

I'm also wondering if more tests need to be added in AdminPinActivityTest if you were able to remove this without any other tests failing besides the ones you fixed. If that's the case then we should add or update tests in that file to validate that the submit button correctly gets disabled when there's an error and re-enables when the user updates the input pin text after an error, including for configuration changes (rotations).

Copy link
Copy Markdown
Contributor Author

@saimanoharigodavarty saimanoharigodavarty Apr 8, 2026

Choose a reason for hiding this comment

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

Thanks for that! The button state logic wasn't removed—it's now centralized with better structure. Here's what changed:

I created an updateSubmitButtonState() method that disables the submit button whenever any PIN validation error exists. This gets called:

  • When the user modifies either PIN field (so the button automatically re-enables once they fix the error)
  • Immediately after validation fails (so the button correctly reflects the error state)

I've also added tests throughout AdminPinActivityTest to verify the button disables on error and re-enables when the user corrects the input—including cases where the configuration changes (device rotation).

This approach ensures the button state always stays in sync with the error state, which prevents accidental submissions of invalid data.

}
2 changes: 1 addition & 1 deletion app/src/main/java/org/oppia/android/app/profile/AdminPinViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ class AdminPinViewModel @Inject constructor() : ObservableViewModel() {
val confirmPinErrorMsg = ObservableField("")
val savedPin = ObservableField("")
val savedConfirmPin = ObservableField("")
val isButtonActive = ObservableField(false)
val isButtonActive = ObservableField(true)
}
33 changes: 27 additions & 6 deletions app/src/sharedTest/java/org/oppia/android/app/profile/AdminPinActivityTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ class AdminPinActivityTest {
}

@Test
fun testAdminPinActivity_inputShortPin_clickIsDisabled() {
fun testAdminPinActivity_inputShortPin_clickIsEnabled() {
launch<AdminPinActivity>(
AdminPinActivity.createAdminPinActivityIntent(
context = context,
Expand All @@ -344,7 +344,28 @@ class AdminPinActivityTest {
closeSoftKeyboard()
)
onView(withId(R.id.submit_button)).perform(nestedScrollTo())
onView(withId(R.id.submit_button)).check(matches(not(isClickable())))
onView(withId(R.id.submit_button)).check(matches(isClickable()))
}
}

@Test
fun testAdminPinActivity_emptyPins_submit_showsPinLengthError() {
Copy link
Copy Markdown
Member

@BenHenning BenHenning Mar 30, 2026

Choose a reason for hiding this comment

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

It would be nice to also assert that the submit button is no longer clickable in this case.

Copy link
Copy Markdown
Contributor Author

@saimanoharigodavarty saimanoharigodavarty Apr 8, 2026

Choose a reason for hiding this comment

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

You're absolutely right. I've updated the test to assert that the submit button is disabled (not clickable) when the pin length error is shown. This validates the fix properly.

launch<AdminPinActivity>(
AdminPinActivity.createAdminPinActivityIntent(
context = context,
profileId = 0,
colorRgb = -10710042,
adminPinEnum = 0
)
).use {
onView(withId(R.id.submit_button)).perform(nestedScrollTo(), click())
onView(withId(R.id.admin_pin_input_pin)).check(
matches(
hasErrorText(
context.resources.getString(R.string.admin_pin_error_pin_length)
)
)
)
}
}

Expand Down Expand Up @@ -717,7 +738,7 @@ class AdminPinActivityTest {
}

@Test
fun testAdminPinActivity_configChange_inputShortPin_submit_clickIsDisabled() {
fun testAdminPinActivity_configChange_inputShortPin_submit_clickIsEnabled() {
launch<AdminPinActivity>(
AdminPinActivity.createAdminPinActivityIntent(
context = context,
Expand All @@ -737,7 +758,7 @@ class AdminPinActivityTest {
closeSoftKeyboard()
)
onView(withId(R.id.submit_button)).perform(nestedScrollTo())
onView(withId(R.id.submit_button)).check(matches(not(isClickable())))
onView(withId(R.id.submit_button)).check(matches(isClickable()))
}
}

Expand Down Expand Up @@ -1040,7 +1061,7 @@ class AdminPinActivityTest {
}

@Test
fun testAdminPinActivity_inputShortPin_configChange_clickIsDisabled() {
fun testAdminPinActivity_inputShortPin_configChange_clickIsEnabled() {
launch<AdminPinActivity>(
AdminPinActivity.createAdminPinActivityIntent(
context = context,
Expand All @@ -1060,7 +1081,7 @@ class AdminPinActivityTest {
)
onView(withId(R.id.submit_button)).perform(nestedScrollTo())
onView(isRoot()).perform(orientationLandscape())
onView(withId(R.id.submit_button)).check(matches(not(isClickable())))
onView(withId(R.id.submit_button)).check(matches(isClickable()))
}
}

Expand Down
Loading