Skip to content
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

Fixes #4294 'added profile deleted message' #4560

Closed
wants to merge 26 commits into from
Closed

Fixes #4294 'added profile deleted message' #4560

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
69bbc79
added profile deleted message
mbobiosio Sep 6, 2022
b386cdb
fixed classes in alphabetical order
mbobiosio Sep 7, 2022
f1e3428
added kdoc
mbobiosio Sep 8, 2022
5ff1077
reviewed comments
mbobiosio Sep 9, 2022
a92c3ed
outside touch for dialog
mbobiosio Sep 9, 2022
1d634af
updated tag
mbobiosio Sep 13, 2022
7882cf7
set outside cancelable to true
mbobiosio Sep 13, 2022
5daf12d
updated tag id
mbobiosio Sep 15, 2022
c0f91a2
added new tests
mbobiosio Sep 16, 2022
3492144
added new tests
mbobiosio Sep 16, 2022
4793299
added new tests
mbobiosio Sep 16, 2022
047c6e0
updated KDoc for tag
mbobiosio Sep 20, 2022
c1274d4
excluded DeleteProfileSuccessDialogFragment in test_file_exemptions.t…
mbobiosio Sep 21, 2022
96d628d
completed unit testing
mbobiosio Sep 23, 2022
2508427
updated comment for new instance
mbobiosio Sep 29, 2022
fa4b79b
removed 'check' from name
mbobiosio Sep 30, 2022
80f8b48
removed unnecessary space
mbobiosio Sep 30, 2022
df71f74
injected AppLanguageResourceHandler for dialog
mbobiosio Sep 30, 2022
af3f9f0
closing dialog and validating activity is running
mbobiosio Oct 4, 2022
d4d176e
closing dialog and validating activity is running -fixed ktlint
mbobiosio Oct 4, 2022
67eca56
added activity state check after closing dialog
mbobiosio Oct 4, 2022
dc9cdb6
removed activitytestrule
mbobiosio Oct 5, 2022
958357d
Merge branch 'develop' into delete-profile-message
mbobiosio Oct 5, 2022
4f5947e
removed unnecessary checks
mbobiosio Oct 13, 2022
1c07fef
removed the test to verify intent
mbobiosio Oct 13, 2022
0e361e6
fixed unused imports
mbobiosio Oct 13, 2022
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
5 changes: 3 additions & 2 deletions ...rc/main/java/org/oppia/android/app/settings/profile/DeleteProfileSuccessDialogFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ import org.oppia.android.app.fragment.InjectableDialogFragment
class DeleteProfileSuccessDialogFragment : InjectableDialogFragment() {
rt4914 marked this conversation as resolved.
Show resolved Hide resolved

companion object {
const val TAG = "DELETE_SUCCESS_DIALOG_FRAGMENT"
/** Argument key for Profile Deletion Success Dialog in [ProfileEditFragmentPresenter]. */
const val DELETE_PROFILE_SUCCESS_DIALOG_FRAGMENT_TAG = "DELETE_PROFILE_SUCCESS_DIALOG_FRAGMENT"

/**
* This function returns a new instance of [DeleteProfileSuccessDialogFragment]
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Updated this

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Updated now

Copy link
Member

Choose a reason for hiding this comment

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

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

Expand Down Expand Up @@ -48,7 +49,7 @@ class DeleteProfileSuccessDialogFragment : InjectableDialogFragment() {
}
}
}.create()
rt4914 marked this conversation as resolved.
Show resolved Hide resolved
alertDialog.setCanceledOnTouchOutside(false)
alertDialog.setCanceledOnTouchOutside(true)
return alertDialog
}
}
5 changes: 4 additions & 1 deletion app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditFragmentPresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ class ProfileEditFragmentPresenter @Inject constructor(
{
if (it is AsyncResult.Success) {
DeleteProfileSuccessDialogFragment.createNewInstance()
.showNow(fragment.childFragmentManager, DeleteProfileSuccessDialogFragment.TAG)
.showNow(
fragment.childFragmentManager,
DeleteProfileSuccessDialogFragment.DELETE_PROFILE_SUCCESS_DIALOG_FRAGMENT_TAG
)
}
}
)
Expand Down
72 changes: 56 additions & 16 deletions app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditActivityTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package org.oppia.android.app.settings.profile

import android.app.Application
import android.content.Context
import androidx.annotation.StringRes
import androidx.appcompat.app.AppCompatActivity
import androidx.test.core.app.ActivityScenario.launch
import androidx.test.core.app.ApplicationProvider
Expand All @@ -11,6 +12,7 @@ import androidx.test.espresso.action.ViewActions.scrollTo
import androidx.test.espresso.assertion.ViewAssertions.matches
import androidx.test.espresso.intent.Intents
import androidx.test.espresso.intent.Intents.intended
import androidx.test.espresso.intent.Intents.times
import androidx.test.espresso.intent.matcher.IntentMatchers.hasComponent
import androidx.test.espresso.matcher.RootMatchers.isDialog
import androidx.test.espresso.matcher.ViewMatchers.hasDescendant
Expand Down Expand Up @@ -216,44 +218,76 @@ class ProfileEditActivityTest {
}

@Test
fun testProfileEdit_deleteProfile_checkReturnsToProfileListOnPhoneOrAdminControlOnTablet() {
Copy link
Member

Choose a reason for hiding this comment

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

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).

fun testProfileEdit_deleteProfileSuccessful_dialogIsDisplayed() {
launch<ProfileEditActivity>(
ProfileEditActivity.createProfileEditActivity(
context = context,
profileId = 1
)
).use {
onView(withId(R.id.profile_delete_button)).perform(click())
onView(withText(R.string.profile_edit_delete_dialog_positive))
.inRoot(isDialog())
.perform(click())
testCoroutineDispatchers.runCurrent()
if (context.resources.getBoolean(R.bool.isTablet)) {
intended(hasComponent(AdministratorControlsActivity::class.java.name))
} else {
intended(hasComponent(ProfileListActivity::class.java.name))
}

onView(withId(R.id.profile_delete_button)).perform(click())
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_title)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_message)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_positive)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_negative)
}
}

@Test
fun testProfileEdit_landscape_deleteProfile_checkReturnsProfileListOnTabletAdminControlOnPhone() {
fun testProfileEdit_deleteProfileSuccessful_dialogIsDisplayedLandscape() {
launch<ProfileEditActivity>(
ProfileEditActivity.createProfileEditActivity(
context = context,
profileId = 1
)
).use {
testCoroutineDispatchers.runCurrent()
onView(withId(R.id.profile_delete_button)).perform(click())
onView(isRoot()).perform(orientationLandscape())
onView(withId(R.id.profile_delete_button)).perform(scrollTo()).perform(click())
onView(withText(R.string.profile_edit_delete_dialog_positive))
.inRoot(isDialog())
.perform(click())
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_title)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_message)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_positive)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_negative)
}
}

@Test
fun testProfileEdit_deleteProfileSuccessful_clickCancel() {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
launch<ProfileEditActivity>(
ProfileEditActivity.createProfileEditActivity(
context = context,
profileId = 1
)
).use {
testCoroutineDispatchers.runCurrent()
onView(withId(R.id.profile_delete_button)).perform(click())
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_title)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_message)
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
onView(withText(R.string.profile_edit_delete_dialog_negative)).perform(click())
onView(withId(R.id.profile_delete_button)).check(matches(isDisplayed()))
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
}

@Test
fun testProfileEdit_deleteProfile_checkReturnsToProfileListOnPhoneOrAdminControlOnTablet() {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
launch<ProfileEditActivity>(
ProfileEditActivity.createProfileEditActivity(
context = context,
profileId = 1
)
).use {
testCoroutineDispatchers.runCurrent()
onView(withId(R.id.profile_delete_button)).perform(click())
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_title)
verifyTextInDialog(textInDialogId = R.string.profile_edit_delete_dialog_message)
onView(withText(R.string.profile_edit_delete_dialog_positive)).check(matches(isDisplayed()))
onView(withText(R.string.profile_edit_delete_dialog_positive)).perform(click())
if (context.resources.getBoolean(R.bool.isTablet)) {
intended(hasComponent(AdministratorControlsActivity::class.java.name))
} else {
intended(hasComponent(ProfileListActivity::class.java.name))
intended(hasComponent(ProfileListActivity::class.java.name), times(0))
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -352,6 +386,12 @@ class ProfileEditActivityTest {
}
}

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

Choose a reason for hiding this comment

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

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

.inRoot(isDialog())
.check(matches(isDisplayed()))
}

// TODO(#59): Figure out a way to reuse modules instead of needing to re-declare them.
@Singleton
@Component(
Expand Down
1 change: 1 addition & 0 deletions scripts/assets/test_file_exemptions.textproto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

We actually should have a test for the fragment, too, and this may influence how you test ProfileEditFragment.

ProfileEditFragment's tests technically only care about whether the dialog shows up when expected, and that the cancel/delete buttons do what's expected (i.e. closes the dialog).

DeleteProfileSuccessDialogFragment's tests, on the other hand, should verify properties regarding the fragment itself (such as its word contents, and that the buttons perform the generic behaviors that they're supposed to).

Copy link
Author

Choose a reason for hiding this comment

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

Okay. I will use this insight to start working on it right away.

Copy link
Member

Choose a reason for hiding this comment

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

This is still unresolved.

exempted_file_path: "app/src/main/java/org/oppia/android/app/settings/profile/LoadProfileEditDeletionDialogListener.kt"
exempted_file_path: "app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditActivityPresenter.kt"
exempted_file_path: "app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditDeletionDialogFragment.kt"
Expand Down