Fixes #4294 'added profile deleted message' #4560

mbobiosio · 2022-09-06T15:44:24Z

Explanation

Fixes #4294
This PR includes a dialog to indicate profile deleted status.
I have used a new Fragment that extends InjectableDialogFragment to show the dialog

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

BenHenning

Thanks @mbobiosio. I took a first pass. Please make sure to fix all failing CI checks, including the ones for static checks (which should require KDocs and tests to be added).

Also, please fill out the part of the PR description asking for various screenshots and videos for different cases to demonstrate how this UI looks and behaves. That context is really important to make sure that everything works the way we expect even for cases that we might not normally consider or try during development.

app/src/main/java/org/oppia/android/app/fragment/FragmentComponentImpl.kt

BenHenning · 2022-09-07T07:47:58Z

@mbobiosio please don't resolve reviewers' conversation threads. Responding is sufficient to indicate that you've addressed the comment (otherwise it makes it hard for us to keep track of things across review iterations).

Also, please make sure to address all points in my previous comment before assigning the PR back (unless you get stuck). CI checks still appear to have failures (and haven't actually finished running).

rt4914

@mbobiosio Suggested changes. Thanks.

app/src/main/java/org/oppia/android/app/settings/profile/DeleteProfileSuccessDialogFragment.kt

BenHenning

Thanks @mbobiosio. Took another pass. I think CI checks are still failing--PTAL.

app/src/main/java/org/oppia/android/app/settings/profile/DeleteProfileSuccessDialogFragment.kt

BenHenning

Thanks @mbobiosio, followed up to the comments.

Please also make sure the CI tests are fixed before sending this back for review. I can't approve this until the CI checks are passing.

BenHenning · 2022-09-10T03:16:07Z

app/src/main/java/org/oppia/android/app/settings/profile/DeleteProfileSuccessDialogFragment.kt

-     * This function is responsible for displaying content in DialogFragment.
-     *
-     * @return [DeleteProfileSuccessDialogFragment]: DialogFragment
+     * This function returns a new instance of [DeleteProfileSuccessDialogFragment]


This should actually be the exact wording I suggest, i.e.:

/** Returns a new instance of [DeleteProfileSuccessDialogFragment]. */

Updated this

This doesn't seem updated in the latest version of the PR. Did you push your changes?

Updated now

This still seems unchanged--I see the old documentation wording in the latest changes of the PR.

app/src/main/java/org/oppia/android/app/settings/profile/DeleteProfileSuccessDialogFragment.kt

rt4914

@mbobiosio Please apply changes as per open comments.

BenHenning

Not all comments have been responded to or addressed.

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt

BenHenning

@mbobiosio I don't think the test changes are matching the reference I sent. Please revise.

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt

BenHenning · 2022-10-05T02:46:20Z

Also @mbobiosio this branch needs to be brought up-to-date with the latest changes in develop since there are conflicts to resolve.

updated with develop

BenHenning

Thanks @mbobiosio. Took another pass--PTAL.

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt

BenHenning

Thanks @mbobiosio. I took another scan, and looked at all old comment threads. I think 2 old threads still need to be resolved, and I noticed two things per the latest changes. The remaining comments should be all of the changes needed for this PR to be finished up.

Also, please update to the latest develop and resolve the pending conflicts so that we can get an accurate CI result.

BenHenning · 2022-10-14T06:42:34Z

app/src/main/java/org/oppia/android/app/settings/profile/DeleteProfileSuccessDialogFragment.kt

-     * This function is responsible for displaying content in DialogFragment.
-     *
-     * @return [DeleteProfileSuccessDialogFragment]: DialogFragment
+     * This function returns a new instance of [DeleteProfileSuccessDialogFragment]


This still seems unchanged--I see the old documentation wording in the latest changes of the PR.

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt

BenHenning · 2022-10-14T06:47:16Z

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt

@@ -365,6 +375,12 @@ class ProfileEditActivityTest {
    }
  }

+  private fun verifyTextInDialog(@StringRes textInDialogId: Int) {
+    onView(withText(context.getString(textInDialogId)))


We can just use withId() here, no need for the extra getString.

BenHenning · 2022-10-14T06:48:20Z

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt

@@ -229,45 +231,53 @@ class ProfileEditActivityTest {
  }

  @Test
-  fun testProfileEdit_deleteProfile_checkReturnsToProfileListOnPhoneOrAdminControlOnTablet() {


I finally realized what I thought has been missing from the changed tests: we still need to verify that deletion occurs via the dialog. I think you need to add a test that finishes the deletion and then verifies that the correct destination activity has been routed to (using the same intends logic as this test used).

BenHenning · 2022-10-14T06:48:54Z

scripts/assets/test_file_exemptions.textproto

@@ -371,6 +371,7 @@ exempted_file_path: "app/src/main/java/org/oppia/android/app/recyclerview/StartS
 exempted_file_path: "app/src/main/java/org/oppia/android/app/resumelesson/ResumeLessonActivityPresenter.kt"
 exempted_file_path: "app/src/main/java/org/oppia/android/app/resumelesson/ResumeLessonFragmentPresenter.kt"
 exempted_file_path: "app/src/main/java/org/oppia/android/app/resumelesson/ResumeLessonViewModel.kt"
+exempted_file_path: "app/src/main/java/org/oppia/android/app/settings/profile/DeleteProfileSuccessDialogFragment.kt"


This is still unresolved.

oppiabot · 2022-10-21T07:43:41Z

Hi @mbobiosio, 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!

added profile deleted message

69bbc79

mbobiosio requested review from BenHenning and rt4914 as code owners September 6, 2022 15:44

mbobiosio assigned rt4914 and BenHenning Sep 6, 2022

BenHenning requested changes Sep 7, 2022

View reviewed changes

app/src/main/java/org/oppia/android/app/fragment/FragmentComponentImpl.kt Outdated Show resolved Hide resolved

BenHenning assigned mbobiosio and unassigned BenHenning Sep 7, 2022

fixed classes in alphabetical order

b386cdb

mbobiosio assigned BenHenning and unassigned mbobiosio Sep 7, 2022

BenHenning assigned mbobiosio and unassigned BenHenning Sep 7, 2022

rt4914 suggested changes Sep 7, 2022

View reviewed changes

rt4914 removed their assignment Sep 7, 2022

added kdoc

f1e3428

mbobiosio assigned rt4914 and BenHenning Sep 9, 2022

BenHenning reviewed Sep 9, 2022

View reviewed changes

BenHenning removed their assignment Sep 9, 2022

mbobiosio added 2 commits September 9, 2022 13:43

reviewed comments

5ff1077

outside touch for dialog

a92c3ed

mbobiosio assigned BenHenning Sep 9, 2022

BenHenning requested changes Sep 10, 2022

View reviewed changes

BenHenning removed their assignment Sep 10, 2022

rt4914 suggested changes Sep 12, 2022

View reviewed changes

rt4914 removed their assignment Sep 12, 2022

mbobiosio added 2 commits September 13, 2022 09:14

updated tag

1d634af

set outside cancelable to true

7882cf7

updated comment for new instance

2508427

mbobiosio assigned BenHenning Sep 30, 2022

mbobiosio added 3 commits September 30, 2022 13:16

removed 'check' from name

fa4b79b

removed unnecessary space

80f8b48

injected AppLanguageResourceHandler for dialog

df71f74

BenHenning reviewed Oct 1, 2022

View reviewed changes

BenHenning removed their assignment Oct 1, 2022

mbobiosio added 2 commits October 4, 2022 10:28

closing dialog and validating activity is running

af3f9f0

closing dialog and validating activity is running -fixed ktlint

d4d176e

mbobiosio assigned BenHenning Oct 4, 2022

added activity state check after closing dialog

67eca56

BenHenning requested changes Oct 5, 2022

View reviewed changes

BenHenning removed their assignment Oct 5, 2022

removed activitytestrule

dc9cdb6

mbobiosio assigned BenHenning Oct 5, 2022

Merge branch 'develop' into delete-profile-message

958357d

updated with develop

BenHenning added the TODO(Ben): Revive label Oct 5, 2022

BenHenning reviewed Oct 11, 2022

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt Outdated Show resolved Hide resolved

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt Outdated Show resolved Hide resolved

BenHenning removed their assignment Oct 11, 2022

BenHenning removed the TODO(Ben): Revive label Oct 11, 2022

mbobiosio added 3 commits October 13, 2022 12:19

removed unnecessary checks

4f5947e

removed the test to verify intent

1c07fef

fixed unused imports

0e361e6

mbobiosio assigned BenHenning Oct 13, 2022

BenHenning requested changes Oct 14, 2022

View reviewed changes

BenHenning removed their assignment Oct 14, 2022

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 21, 2022

oppiabot bot closed this Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #4294 'added profile deleted message' #4560

Fixes #4294 'added profile deleted message' #4560

mbobiosio commented Sep 6, 2022 •

edited

Loading

BenHenning left a comment

BenHenning commented Sep 7, 2022

rt4914 left a comment

BenHenning left a comment

BenHenning left a comment

BenHenning Sep 10, 2022

mbobiosio Sep 28, 2022

BenHenning Sep 29, 2022

mbobiosio Sep 29, 2022

BenHenning Oct 14, 2022

rt4914 left a comment

BenHenning left a comment

BenHenning left a comment

BenHenning commented Oct 5, 2022

BenHenning left a comment

BenHenning left a comment

BenHenning Oct 14, 2022

BenHenning Oct 14, 2022

BenHenning Oct 14, 2022

BenHenning Oct 14, 2022

oppiabot bot commented Oct 21, 2022

Fixes #4294 'added profile deleted message' #4560

Fixes #4294 'added profile deleted message' #4560

Conversation

mbobiosio commented Sep 6, 2022 • edited Loading

Explanation

Essential Checklist

For UI-specific PRs only

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Sep 7, 2022

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Oct 5, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 21, 2022

mbobiosio commented Sep 6, 2022 •

edited

Loading