Fixes #4352: Spelling mistake while setting up pin in 'Enter a New Pin' dialog box #4425

pratyaksh1610 · 2022-07-08T09:14:29Z

Explanation

Fixes #4352
Updated the spelling while setting up pin in 'Enter a New Pin' dialog box.

Screenshots -

Before

After

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)
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

rt4914

@pratyaksh1610 PTAL at the suggested changes.

rt4914 · 2022-07-11T13:11:02Z

app/src/main/java/org/oppia/android/app/profile/ResetPinDialogFragmentPresenter.kt

@@ -68,7 +68,9 @@ class ResetPinDialogFragmentPresenter @Inject constructor(
    val dialog = AlertDialog.Builder(activity, R.style.OppiaAlertDialogTheme)
      .setTitle(R.string.reset_pin_enter)
      .setView(binding.root)
-      .setMessage("This PIN wil be $name's new PIN and will be required when signing in.")
+


Remove this extra blank line.

rt4914 · 2022-07-11T13:11:06Z

app/src/main/java/org/oppia/android/app/profile/ResetPinDialogFragmentPresenter.kt

-      .setMessage("This PIN wil be $name's new PIN and will be required when signing in.")
+
+      .setMessage("This PIN will be $name's new PIN and will be required when signing in.")
+


Remove this extra blank line.

pratyaksh1610 · 2022-07-11T15:21:10Z

@rt4914 removed the blank lines. Pls review.

BenHenning

Hi @pratyaksh1610. I caught this in passing, but I think there might be a bit more work needed here. PTAL at my in-file comment.

BenHenning · 2022-07-12T04:07:59Z

app/src/main/java/org/oppia/android/app/profile/ResetPinDialogFragmentPresenter.kt

@@ -68,7 +68,7 @@ class ResetPinDialogFragmentPresenter @Inject constructor(
    val dialog = AlertDialog.Builder(activity, R.style.OppiaAlertDialogTheme)
      .setTitle(R.string.reset_pin_enter)
      .setView(binding.root)
-      .setMessage("This PIN wil be $name's new PIN and will be required when signing in.")
+      .setMessage("This PIN will be $name's new PIN and will be required when signing in.")


This actually should be an app string so that it can be localized. I'm a bit surprised we missed this.

Could you move this to strings.xml? You can see other strings that take variables for a reference on how to pass a value to a localized string.

Also, please add a test to verify that the dialog's message is correct when opened.

pratyaksh1610 · 2022-07-14T08:12:21Z

@BenHenning I have localised the string.
How to add tests ? , I have not done testing before.

pratyaksh1610 · 2022-07-14T18:03:42Z

I am getting error while running existing test, how to resolve this ?

BenHenning · 2022-07-15T04:21:33Z

@BenHenning I have localised the string. How to add tests ? , I have not done testing before.

@pratyaksh1610 I suggest looking at other dialog fragment tests in the codebase for an idea on how to set up such a test, including verifying string contents. You'll need to create a new test file for the reset pin dialog fragment.

I am getting error while running existing test, how to resolve this ?

This seems like a transient Android Studio issue--I suggest creating a new discussion in the discussions board to see if anyone else can help with this (as I'm not sure offhand).

pratyaksh1610 · 2022-07-16T13:45:36Z

@BenHenning Created a new test file ResetPinDialogMessageTest.kt and added test.
Pls review.

BenHenning

Thanks @pratyaksh1610. Apologies for the delayed review. I've taken a pass & left some thoughts--PTAL!

BenHenning · 2022-07-20T21:07:08Z

app/build.gradle

@@ -16,6 +16,7 @@ android {
    versionName "1.0"
    multiDexEnabled true
    testInstrumentationRunner "org.oppia.android.testing.OppiaTestRunner"
+    testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner"


This & the other build.gradle changes shouldn't be needed--please make sure follow existing conventions for how to set up the test correctly (i.e. which annotations to use).

BenHenning · 2022-07-20T21:10:00Z

app/src/sharedTest/java/org/oppia/android/app/profile/ResetPinDialogMessageTest.kt

+import androidx.test.espresso.matcher.ViewMatchers.withId
+import org.junit.Test
+
+class ResetPinDialogMessageTest {


This should be setting up a test activity & opening the dialog under test to verify. See: https://github.com/oppia/oppia-android/blob/develop/app/src/sharedTest/java/org/oppia/android/app/devoptions/ViewEventLogsFragmentTest.kt for a reference. Note that you might be able to use https://github.com/oppia/oppia-android/blob/develop/app/src/main/java/org/oppia/android/app/testing/activity/TestActivity.kt instead of creating a new test activity, too (which will be a bit easier, so that's a good place to start).

BenHenning · 2022-07-20T21:10:45Z

Also @pratyaksh1610 it appears that you have failing CI checks--please make sure to address these as well.

pratyaksh1610 · 2022-07-21T05:55:25Z

@BenHenning So i just need to add that fun check_name_displayed_on_reset_pin_dialog_fragment_is_correct() function which I created in a new test file to TestActivity.kt .

pratyaksh1610 · 2022-07-29T15:59:20Z

@BenHenning For adding a test, should I create a new test activity for that alert dialog and add the function mentioned below in that file ?
Is this the correct way .

  @Test
  fun check_name_displayed_on_reset_pin_dialog_fragment_is_correct(){
    Espresso.onView(withId(R.string.reset_pin_enter_dialog_message)).check(
      ViewAssertions.matches(
        ViewMatchers.isDisplayed()
      )
    )
  }

BenHenning · 2022-07-29T23:42:28Z

@BenHenning For adding a test, should I create a new test activity for that alert dialog and add the function mentioned below in that file ? Is this the correct way .
  @Test
  fun check_name_displayed_on_reset_pin_dialog_fragment_is_correct(){
    Espresso.onView(withId(R.string.reset_pin_enter_dialog_message)).check(
      ViewAssertions.matches(
        ViewMatchers.isDisplayed()
      )
    )
  }

@pratyaksh1610 yep you'll need to create a new test activity to host the dialog fragment, and then intent to it in a new test suite in order to bring up the dialog in order to test it.

pratyaksh1610 · 2022-08-01T17:47:49Z

@BenHenning I have made the changes. Please review. Thanks.

pratyaksh1610 · 2022-08-02T07:25:59Z

Test passed

rt4914

@pratyaksh1610 LGTM

rt4914 · 2022-08-02T13:30:00Z

app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt

+          withId(R.id.admin_settings_input_pin_edit_text),
+          isDescendantOfA(withId(R.id.admin_settings_input_pin))
+        )
+      ).inRoot(isDialog())


I think the formatting here can be improved otherwise the PR looks good.

BenHenning · 2022-08-03T05:53:15Z

@pratyaksh1610 sorry but I need to close this. See point (4) https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#making-a-local-code-change-using-the-terminal. Since this PR was force-pushed, it'll need to be recreated. Please avoid force pushing in the future as it breaks comment history and makes incremental reviews much more difficult (which is why we have a no-allow policy for force pushing.

pratyaksh1610 · 2022-08-03T06:52:22Z

@BenHenning I have made a new PR for the same. Please review.

pratyaksh1610 requested a review from rt4914 as a code owner July 8, 2022 09:14

pratyaksh1610 changed the title ~~Fixes #4352 : Spelling mistake while setting up pin in 'Enter a New Pin' dialog box~~ Fixes #4352: Spelling mistake while setting up pin in 'Enter a New Pin' dialog box Jul 8, 2022

vrajdesai78 self-requested a review July 8, 2022 16:52

vrajdesai78 approved these changes Jul 8, 2022

View reviewed changes

vrajdesai78 assigned rt4914 Jul 8, 2022

rt4914 suggested changes Jul 11, 2022

View reviewed changes

rt4914 assigned pratyaksh1610 and unassigned rt4914 Jul 11, 2022

BenHenning requested changes Jul 12, 2022

View reviewed changes

BenHenning self-assigned this Jul 19, 2022

BenHenning requested changes Jul 20, 2022

View reviewed changes

BenHenning removed their assignment Jul 20, 2022

updated

0dfd88b

pratyaksh1610 added 2 commits August 1, 2022 23:02

Merge branch 'oppia:develop' into update-spelling

f0cfc87

added test

d3cfc67

updated test

32c5fbb

vrajdesai78 assigned rt4914 and unassigned pratyaksh1610 Aug 2, 2022

rt4914 approved these changes Aug 2, 2022

View reviewed changes

rt4914 assigned BenHenning and pratyaksh1610 and unassigned rt4914 Aug 2, 2022

BenHenning closed this Aug 3, 2022

pratyaksh1610 deleted the update-spelling branch August 3, 2022 06:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #4352: Spelling mistake while setting up pin in 'Enter a New Pin' dialog box #4425

Fixes #4352: Spelling mistake while setting up pin in 'Enter a New Pin' dialog box #4425

pratyaksh1610 commented Jul 8, 2022

rt4914 left a comment

rt4914 Jul 11, 2022

pratyaksh1610 Jul 11, 2022

rt4914 Jul 11, 2022

pratyaksh1610 commented Jul 11, 2022

BenHenning left a comment

BenHenning Jul 12, 2022

pratyaksh1610 commented Jul 14, 2022

pratyaksh1610 commented Jul 14, 2022

BenHenning commented Jul 15, 2022 •

edited

Loading

pratyaksh1610 commented Jul 16, 2022

BenHenning left a comment

BenHenning Jul 20, 2022

BenHenning Jul 20, 2022

BenHenning commented Jul 20, 2022

pratyaksh1610 commented Jul 21, 2022

pratyaksh1610 commented Jul 29, 2022

BenHenning commented Jul 29, 2022

pratyaksh1610 commented Aug 1, 2022

pratyaksh1610 commented Aug 2, 2022

rt4914 left a comment

rt4914 Aug 2, 2022

BenHenning commented Aug 3, 2022

pratyaksh1610 commented Aug 3, 2022 •

edited

Loading

		.setMessage("This PIN wil be $name's new PIN and will be required when signing in.")

		.setMessage("This PIN will be $name's new PIN and will be required when signing in.")

Fixes #4352: Spelling mistake while setting up pin in 'Enter a New Pin' dialog box #4425

Fixes #4352: Spelling mistake while setting up pin in 'Enter a New Pin' dialog box #4425

Conversation

pratyaksh1610 commented Jul 8, 2022

Explanation

Screenshots -

Before

After

Essential Checklist

For UI-specific PRs only

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 Jul 11, 2022

Choose a reason for hiding this comment

pratyaksh1610 Jul 11, 2022

Choose a reason for hiding this comment

rt4914 Jul 11, 2022

Choose a reason for hiding this comment

pratyaksh1610 commented Jul 11, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Jul 12, 2022

Choose a reason for hiding this comment

pratyaksh1610 commented Jul 14, 2022

pratyaksh1610 commented Jul 14, 2022

BenHenning commented Jul 15, 2022 • edited Loading

pratyaksh1610 commented Jul 16, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Jul 20, 2022

Choose a reason for hiding this comment

BenHenning Jul 20, 2022

Choose a reason for hiding this comment

BenHenning commented Jul 20, 2022

pratyaksh1610 commented Jul 21, 2022

pratyaksh1610 commented Jul 29, 2022

BenHenning commented Jul 29, 2022

pratyaksh1610 commented Aug 1, 2022

pratyaksh1610 commented Aug 2, 2022

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 Aug 2, 2022

Choose a reason for hiding this comment

BenHenning commented Aug 3, 2022

pratyaksh1610 commented Aug 3, 2022 • edited Loading

BenHenning commented Jul 15, 2022 •

edited

Loading

pratyaksh1610 commented Aug 3, 2022 •

edited

Loading