Fix #2427: Create LogoutDialogFragment #3119

ArpitShukIa · 2021-04-26T06:21:55Z

Explanation

Fixes #2427: I created a new DialogFragment replacing the old AlertDialog in AdministratorControlsAccountActionsViewModel and removed the IntentFactoryShim dependency as it is no longer being used. Also added test for the fix.

Test Results:

Robolectric:

Expresso:

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

ArpitShukIa · 2021-04-26T06:22:29Z

@rt4914 PTAL.

ArpitShukIa · 2021-04-26T06:37:50Z

The bazel checks are failing. Probably because LogoutDialogFragment is not visible to AdministratorControlsAccountActionsViewModel.
How to fix this?

rt4914 · 2021-04-26T07:21:32Z

Will review it once it gets review by @prayutsu or @FareesHussain

prayutsu

Left some comments. PTAL @ArpitShukIa

...rcontrols/administratorcontrolsitemviewmodel/AdministratorControlsAccountActionsViewModel.kt

app/src/main/java/org/oppia/android/app/administratorcontrols/LogoutDialogFragment.kt

FareesHussain

@ArpitShukIa Implementation LGTM,

PTAL there is a bazel build error.

As AdministratorControlsAccountActionsViewModel no longer imports the resources move it from VIEW_MODELS_WITH_RESOURCE_IMPORTS to VIEW_MODELS source set to optimize the build but this doesn't solve the bazel error, as we don't have Activities or Fragments in the viewmodels so far there is no dep added to the view_models target.

To fix this we need to add the :app dep to the :view_models but this is definitely not a good practice.

I suggest creating a new Listner and use the LogoutDialogFragment within the AdministratorControlsActivity or AdministratorControlsFragment

@BenHenning WDYT?

...rcontrols/administratorcontrolsitemviewmodel/AdministratorControlsAccountActionsViewModel.kt

FareesHussain

LGTM, Thanks

prayutsu

LGTM, Thanks @ArpitShukIa

BenHenning · 2021-04-27T21:34:57Z

Sorry, I think I'll need to take a pass on this tomorrow, but I can follow up to the comment above now.

BenHenning · 2021-04-27T21:41:27Z

@ArpitShukIa Implementation LGTM,

PTAL there is a bazel build error.

As AdministratorControlsAccountActionsViewModel no longer imports the resources move it from VIEW_MODELS_WITH_RESOURCE_IMPORTS to VIEW_MODELS source set to optimize the build but this doesn't solve the bazel error, as we don't have Activities or Fragments in the viewmodels so far there is no dep added to the view_models target.

To fix this we need to add the :app dep to the :view_models but this is definitely not a good practice.

I suggest creating a new Listner and use the LogoutDialogFragment within the AdministratorControlsActivity or AdministratorControlsFragment

@BenHenning WDYT?

I agree with moving this out of the resources view model list if R is no longer needed. Regarding the dependency, I'm fairly certain you can't add :app to :view_models since that will likely result in a circular dependency.

I'm not sure I'm following what the need for the listener is. What specific problem do you see here @FareesHussain?

FareesHussain · 2021-04-28T05:42:44Z

I'm not sure I'm following what the need for the listener is. What specific problem do you see here @FareesHussain?

https://github.com/oppia/oppia-android/runs/2445936672

Before using the listener. the AdministratorControlsAccountActionsViewModel imports the LogoutDialogFragment. I had a look into the error and found that the LogoutDialogFragment comes under the :app target, I noticed none of the other viewmodels have imported from the :app target so to fix the Bazel error, I suggested adding a listener inorder to call the showLogoutDialog function. I'm not sure if there is any other way to fix it.

BenHenning · 2021-04-29T00:58:15Z

Ah I see, thanks for clarifying @FareesHussain. Yes, that seems like the correct way to solve this since the interface provides indirection (plus it's a bit cleaner not to directly interact with the activity/fragment lifecycles except at in that activity or fragment implementation class).

BenHenning · 2021-04-29T00:58:48Z

I'm actually going to take a pass on just my codeowners here (presumably Bazel) after @rt4914 has a chance to review it in case there are any big changes.

rt4914

LGTM, thanks.

rt4914 · 2021-04-30T07:23:58Z

app/src/main/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivity.kt

@@ -16,14 +16,16 @@ const val SELECTED_CONTROLS_TITLE_SAVED_KEY =
 const val LAST_LOADED_FRAGMENT_KEY = "LAST_LOADED_FRAGMENT_KEY"
 const val PROFILE_LIST_FRAGMENT = "PROFILE_LIST_FRAGMENT"
 const val APP_VERSION_FRAGMENT = "APP_VERSION_FRAGMENT"
+const val LOGOUT_DIALOG_FRAGMENT = "LOGOUT_DIALOG_FRAGMENT"


This tag should be shifted to LogoutDialogFragment companion object and then it should be used wherever requried.

rt4914 · 2021-04-30T07:25:45Z

app/src/main/java/org/oppia/android/app/administratorcontrols/LogoutDialogFragment.kt

+        dialog.dismiss()
+      }
+      .setPositiveButton(R.string.log_out_dialog_okay_button) { _, _ ->
+        // TODO(#762): Replace [ProfileChooserActivity] to [LoginActivity] once it is added.


This TODO is not needed here. You can remove this.

BenHenning

Thanks @ArpitShukIa! Generally LGTM for Bazel changes, but had 2 nit comments to address before approving.

app/src/main/java/org/oppia/android/app/administratorcontrols/ShowLogoutDialogListener.kt

ArpitShukIa · 2021-05-05T07:06:39Z

@BenHenning I have committed the requested changes. PTAL.

BenHenning

Thanks @ArpitShukIa. LGTM for codeowners.

BenHenning · 2021-05-06T09:06:36Z

The Bazel issues are existing problems and are unrelated to this PR, so merging this as-is.

Create LogoutDialogFragment

c0681f8

ArpitShukIa requested a review from rt4914 as a code owner April 26, 2021 06:21

ArpitShukIa assigned rt4914 Apr 26, 2021

rt4914 assigned prayutsu and FareesHussain Apr 26, 2021

rt4914 removed their assignment Apr 26, 2021

prayutsu suggested changes Apr 27, 2021

View reviewed changes

...rcontrols/administratorcontrolsitemviewmodel/AdministratorControlsAccountActionsViewModel.kt Outdated Show resolved Hide resolved

app/src/main/java/org/oppia/android/app/administratorcontrols/LogoutDialogFragment.kt Show resolved Hide resolved

prayutsu assigned ArpitShukIa and unassigned prayutsu Apr 27, 2021

Add companion object

0355198

ArpitShukIa assigned prayutsu and unassigned ArpitShukIa Apr 27, 2021

FareesHussain suggested changes Apr 27, 2021

View reviewed changes

...rcontrols/administratorcontrolsitemviewmodel/AdministratorControlsAccountActionsViewModel.kt Outdated Show resolved Hide resolved

FareesHussain assigned BenHenning and ArpitShukIa and unassigned FareesHussain Apr 27, 2021

Introduce ShowLogoutDialogListener

9b3050d

ArpitShukIa requested a review from BenHenning as a code owner April 27, 2021 11:18

FareesHussain unassigned ArpitShukIa Apr 27, 2021

FareesHussain approved these changes Apr 27, 2021

View reviewed changes

prayutsu approved these changes Apr 27, 2021

View reviewed changes

prayutsu assigned rt4914 and unassigned prayutsu Apr 27, 2021

BenHenning assigned FareesHussain and unassigned BenHenning Apr 27, 2021

FareesHussain assigned BenHenning and unassigned FareesHussain Apr 28, 2021

Use "log out" instead of "logout"

11046a5

rt4914 approved these changes Apr 30, 2021

View reviewed changes

rt4914 removed their assignment Apr 30, 2021

ArpitShukIa added 2 commits April 30, 2021 14:28

nit

c5d60e2

nit

5d2abc6

BenHenning requested changes May 1, 2021

View reviewed changes

app/src/main/java/org/oppia/android/app/administratorcontrols/ShowLogoutDialogListener.kt Outdated Show resolved Hide resolved

app/src/main/java/org/oppia/android/app/administratorcontrols/ShowLogoutDialogListener.kt Show resolved Hide resolved

BenHenning assigned ArpitShukIa and unassigned BenHenning May 1, 2021

nit

38acea0

ArpitShukIa assigned BenHenning and unassigned ArpitShukIa May 1, 2021

BenHenning approved these changes May 6, 2021

View reviewed changes

BenHenning merged commit a623929 into oppia:develop May 6, 2021

ArpitShukIa deleted the logout-dialog branch May 7, 2021 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2427: Create LogoutDialogFragment #3119

Fix #2427: Create LogoutDialogFragment #3119

ArpitShukIa commented Apr 26, 2021

ArpitShukIa commented Apr 26, 2021

ArpitShukIa commented Apr 26, 2021

rt4914 commented Apr 26, 2021

prayutsu left a comment

FareesHussain left a comment

FareesHussain left a comment

prayutsu left a comment

BenHenning commented Apr 27, 2021 •

edited

BenHenning commented Apr 27, 2021

FareesHussain commented Apr 28, 2021

BenHenning commented Apr 29, 2021

BenHenning commented Apr 29, 2021 •

edited

rt4914 left a comment

rt4914 Apr 30, 2021

ArpitShukIa Apr 30, 2021

rt4914 Apr 30, 2021

ArpitShukIa Apr 30, 2021

BenHenning left a comment

ArpitShukIa commented May 5, 2021

BenHenning left a comment

BenHenning commented May 6, 2021

Fix #2427: Create LogoutDialogFragment #3119

Fix #2427: Create LogoutDialogFragment #3119

Conversation

ArpitShukIa commented Apr 26, 2021

Explanation

Test Results:

Checklist

ArpitShukIa commented Apr 26, 2021

ArpitShukIa commented Apr 26, 2021

rt4914 commented Apr 26, 2021

prayutsu left a comment

Choose a reason for hiding this comment

FareesHussain left a comment

Choose a reason for hiding this comment

FareesHussain left a comment

Choose a reason for hiding this comment

prayutsu left a comment

Choose a reason for hiding this comment

BenHenning commented Apr 27, 2021 • edited

BenHenning commented Apr 27, 2021

FareesHussain commented Apr 28, 2021

BenHenning commented Apr 29, 2021

BenHenning commented Apr 29, 2021 • edited

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 Apr 30, 2021

Choose a reason for hiding this comment

ArpitShukIa Apr 30, 2021

Choose a reason for hiding this comment

rt4914 Apr 30, 2021

Choose a reason for hiding this comment

ArpitShukIa Apr 30, 2021

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

ArpitShukIa commented May 5, 2021

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented May 6, 2021

BenHenning commented Apr 27, 2021 •

edited

BenHenning commented Apr 29, 2021 •

edited