Fix Part of #4938: Create Admin PIN (4/12) by adhiamboperes · Pull Request #5880 · oppia/oppia-android

adhiamboperes · 2025-07-04T15:13:34Z

Explanation

Fixes part of #4938

Introduces a new screen for creating an admin PIN during admin onboarding. This is the last step of admin onboarding.

The PR ensures there are validations to enforce correct PINs that are 5 digits long and that the user inputs and appropriate error messages are retained even on configuration changes.

There are additional changes that clean up the onboarding flow placeholders, and tests have been added where necessary.

Essential Checklist

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

# Conflicts: # app/src/main/res/values/styles.xml

…n-pin-input

github-actions · 2026-04-01T22:43:41Z

@adhiamboperes this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text.

adhiamboperes · 2026-04-01T22:52:55Z

Also the issue assignment workflow is still failing despite this branch being up to date with develop.

BenHenning · 2026-04-01T23:20:21Z

Also the issue assignment workflow is still failing despite this branch being up to date with develop.

@adhiamboperes it seems the regex check is case sensitive. Perhaps it will pass now?

BenHenning

Thanks @adhiamboperes! Took a full pass, PTAL. I'll also respond to my earlier long comment (but can't as part of this review due to a GitHub bug).

BenHenning · 2026-04-03T04:21:34Z

+
+  @Test
+  fun testFragment_onLaunch_allTextViewsHaveCorrectContent() {
+    launch(CreateAdminPinActivity::class.java).use {


Nit: it's ideal to test fragments in the context of a different activity. If it's easy I suggest using TestActivity for this purpose.

That might potentially help with figuring out the timeout, too, since it looks like (per CI) both the fragment and activity classes are hanging.

BenHenning · 2026-04-03T04:27:34Z

+      delay(100)
+      keyboardController?.show()


Is the delay and explicit show actually necessary? Gemini seems to suggest that the delay is likely what's causing the deadlock with our coroutine dispatcher testing barrier which would cause the hanging. It seems to assume that these lines may be unnecessary because focusing the text view ought to be sufficient to open the keyboard automatically.

However, if this is called too soon it may need to be wrapped in a onGloballyPositioned so that focus happens after layout.

No, They keyboard shows correctly without the delay, so I have removed it, and there was no need to use onGloballyPositioned

Removing the delay does not fix the hanging test issue. I had previously spent a lot of time investigating this issue, and the closest I got was pinpointing a platform parameter load issue:

at org.oppia.android.testing.platformparameter.TestPlatformParameterModule$providePlatformParameterController$1.loadParametersAsync at org.oppia.android.app.activity.InjectableAppCompatActivity$ensurePlatformParametersAreLoaded$1.invokeSuspend

BenHenning · 2026-04-03T04:29:54Z

+            {
+              focusManager.clearFocus()
+              onDone?.invoke()
+            }


Unnecessary extra curly braces?

These are needed. The function expectes a unit:

Error: type mismatch: inferred type is Unit? but (KeyboardActionScope.() -> Unit)? was expected onDone = if (imeAction == ImeAction.Done) { ^

BenHenning · 2026-04-03T04:33:43Z

+      else -> PinValidationResult(
+        isValid = false,
+        errorMessage = resourceHandler.getStringInLocaleWithWrapping(
+          R.string.create_admin_pin_activity_mismatch_error


There are some repeat references to these strings. Is it possible to cleanly refactor to avoid repeating these such that each string is only referenced once?

Added these to the companion object and refactored the references.

BenHenning · 2026-04-03T04:36:11Z

+  private data class PinValidationResult(
+    val isValid: Boolean,
+    val errorMessage: String = ""
+  )


Optional, but you could refine this a bit by leveraging a sealed class and also using a StringRes so that the string lookup only needs to happen in one place for the error:

private sealed class PinValidationResult { object Valid: PinValidationResult() data class Invalid(@StringRes val errorMessageId: Int): PinValidationResult() }

Done, the code readability has improved a bit.

BenHenning · 2026-04-03T04:37:21Z

+  private val resourceHandler: AppLanguageResourceHandler,
+  private val profileManagementController: ProfileManagementController
+) {
+  private lateinit var binding: CreateAdminPinFragmentBinding


Does this actually need to use databinding? It seems unnecessary looking at the layout file (and probably better to avoid it since I'm not exactly sure how well Compose and databinding will interoperate...).

I have made the ComposeView the root element in the layout, and refactored its reference directly:

val composeView = inflater.inflate( R.layout.create_admin_pin_fragment, container, /* attachToRoot= */ false ) as ComposeView

BenHenning · 2026-04-03T04:39:00Z

    }
  }

+  fun testFragment_supervisorOnboardingFlow_stepCountThreeText_isDisplayed() {


Missing @Test.

BenHenning · 2026-04-03T04:42:05Z

+      testCoroutineDispatchers.runCurrent()
+
+      composeRule
+        .onNodeWithText(context.getString(R.string.create_admin_pin_activity_blank_error))


Hmm looking at these tests, I acknowledge they are quite clean however one issue I see is that there's a bit of a risk here. I think it's theoretically possible to show every related string to the new screen at one time and all of the tests will pass (or most will, there are a few assert not visible).

This is actually why I think it's important to validate specific views but I know Compose makes that tricky. I think we do need a solution for this but I'm not precisely sure what it is.

I realized that I didn't quite understand your intent here.

If you mean that we should assert something about the structure or context of those strings, then we could try to ensure we assert for a few conditions at a time, e.g. all other errors hidden + button disabled + only expected error shown. The downside is that the tests can be overly verbose.

But this doesn't look unique to Compose, since the problem could still occur with Android Views.

Or did you mean:

composeTestRule .onNode(matcher).assert(hasText("Button") or hasText("Button2"))

BenHenning · 2026-04-03T04:44:40Z

+  // The submit-time error message to display when show_error is true.
+  string error_message = 4;


Will this cause any problems with the user changing their system language while the activity is open?

No, it won't. I tested this and the string language updates correctly for all errors on activity recreation.

Screen_recording_20260421_160640.webm

BenHenning · 2026-04-03T04:45:44Z

+exempted_file_path: "app/src/main/java/org/oppia/android/app/profile/CreateAdminPinActivityPresenter.kt"
+test_file_not_required: true


Nit: missing spacing.

This file does not use spacing between each item.

oppiabot · 2026-04-03T04:56:29Z

Unassigning @BenHenning since the review is done.

oppiabot · 2026-04-03T04:56:32Z

Hi @adhiamboperes, it looks like some changes were requested on this pull request by @BenHenning. PTAL. Thanks!

…pin-setup

# Conflicts: # app/src/main/java/org/oppia/android/app/onboarding/AdminIntroFragmentPresenter.kt # app/src/main/java/org/oppia/android/app/profile/ProfileChooserFragmentPresenter.kt # model/src/main/proto/screens.proto # utility/src/main/java/org/oppia/android/util/logging/EventBundleCreator.kt

oppiabot · 2026-09-07T15:21:45Z

Hi @adhiamboperes, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you! If you're unsure how to reassign this PR to a reviewer, please make sure to review the wiki page that details the Guidance on submitting PRs.

adhiamboperes and others added 30 commits March 19, 2025 02:13

New layout for auth

f3e8961

Merge remote-tracking branch 'upstream/develop' into redesign-pin-input

471e1a6

# Conflicts: # app/src/main/res/values/styles.xml

Add compose livedata dependency

14296af

Create color and string resources

cb7936f

Create xml layouts

b10ae81

Create ProfileLoginActivity

5a366f1

Create ProfileLoginFragment

83f3762

Refactor login intent to new ProfileLoginActivity

c384747

Add todos to track cleanup

a691304

Cleanup unused items

eec9434

Add admin reset pin flow

99e1a19

Add route to non-admin reset pin flow

a93542a

Add non-admin pin reset flow

1ea182a

wip commit

69973d1

Merge branch 'develop' of github.com:oppia/oppia-android into redesig…

b393870

…n-pin-input

Create AdminIntroActivity

f00119d

Create AdminIntroFragment

8d6139a

Correct ktlint errors

2796aa4

Add navigation to the screen

3cff4d1

Create the screen components

9145b20

Add navigation to the next screen

44d77bf

Create PinSetupActivity

1fbb75a

Create PinSetupFragment

726fd88

Add navigation and todos to track cleanup

540d7e4

Cleanup lint issues

6fbe1cc

Add activity tests

9ef0501

Add fragment tests

734c9ab

Break down large composables

d894646

Add tests for login flow

a0ca9de

Move dialogs to own files

0524005

github-actions Bot marked this pull request as draft April 1, 2026 22:43

adhiamboperes marked this pull request as ready for review April 1, 2026 22:54

BenHenning requested changes Apr 3, 2026

View reviewed changes

oppiabot Bot unassigned BenHenning Apr 3, 2026

oppiabot Bot assigned adhiamboperes Apr 3, 2026

This comment was marked as outdated.

Sign in to view

oppiabot Bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 10, 2026

adhiamboperes removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 14, 2026

adhiamboperes added 2 commits April 21, 2026 00:30

Merge branch 'develop' of github.com:oppia/oppia-android into create-…

4984ef2

…pin-setup

address reviewer comments

0234f18

Sandesh282 mentioned this pull request Apr 21, 2026

Fix #5886: Optimize lint execution #6098

Merged

6 tasks

This comment was marked as outdated.

Sign in to view

oppiabot Bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 28, 2026

oppiabot Bot closed this May 5, 2026

Refine compose logic

eaaabb2

adhiamboperes reopened this May 11, 2026

adhiamboperes added 2 commits May 12, 2026 03:11

Try to synchronize tests

d7aa017

Fix lint issues

ae8f2d8

This comment was marked as outdated.

Sign in to view

adhiamboperes mentioned this pull request Sep 2, 2026

Fix #6145: Fix Admin PIN tablet layout #6370

Open

7 tasks

		// The submit-time error message to display when show_error is true.
		string error_message = 4;

		exempted_file_path: "app/src/main/java/org/oppia/android/app/profile/CreateAdminPinActivityPresenter.kt"
		test_file_not_required: true

Uh oh!

Conversation

adhiamboperes commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Essential Checklist

For UI-specific PRs only

Uh oh!

github-actions Bot commented Apr 1, 2026

Uh oh!

adhiamboperes commented Apr 1, 2026

Uh oh!

BenHenning commented Apr 1, 2026

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adhiamboperes Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oppiabot Bot commented Apr 3, 2026

Uh oh!

oppiabot Bot commented Apr 3, 2026

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

oppiabot Bot commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

adhiamboperes commented Jul 4, 2025 •

edited

Loading

adhiamboperes Apr 21, 2026 •

edited

Loading