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

Show banner after editing finalized form #5762

Merged
merged 7 commits into from
Oct 13, 2023
Merged

Show banner after editing finalized form #5762

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fc3d35e
Genericize deprectation banner
seadowg Oct 3, 2023
6468094
Show banner after editing finalized form
seadowg Oct 3, 2023
da66779
Extract constant for key
seadowg Oct 3, 2023
bcd3630
Move analytics event
seadowg Oct 3, 2023
e8cdc89
Update test
seadowg Oct 3, 2023
7a0f0bd
Update copy
seadowg Oct 3, 2023
2665ddc
Make both warnings match
seadowg Oct 3, 2023
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
26 changes: 22 additions & 4 deletions ...roidTest/java/org/odk/collect/android/feature/instancemanagement/SendFinalizedFormTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package org.odk.collect.android.feature.instancemanagement

import androidx.test.espresso.intent.Intents.intended
import androidx.test.espresso.intent.matcher.IntentMatchers.hasComponent
import androidx.test.espresso.intent.matcher.IntentMatchers.hasExtra
import androidx.test.ext.junit.runners.AndroidJUnit4
import org.hamcrest.CoreMatchers.allOf
import org.junit.Rule
import org.junit.Test
import org.junit.rules.RuleChain
import org.junit.runner.RunWith
import org.odk.collect.android.activities.WebViewActivity
import org.odk.collect.android.support.CollectHelpers.addGDProject
import org.odk.collect.android.support.TestDependencies
import org.odk.collect.android.support.pages.FormEntryPage.QuestionAndAnswer
Expand All @@ -17,6 +22,7 @@ import org.odk.collect.android.support.rules.CollectTestRule
import org.odk.collect.android.support.rules.TestRuleChain.chain
import org.odk.collect.androidtest.RecordedIntentsRule
import org.odk.collect.projects.Project.New
import org.odk.collect.strings.R.string

@RunWith(AndroidJUnit4::class)
class SendFinalizedFormTest {
Expand All @@ -43,7 +49,19 @@ class SendFinalizedFormTest {
.answerQuestion("what is your age", "53")
.swipeToEndScreen()
.clickFinalize()
.checkIsSnackbarWithMessageDisplayed(org.odk.collect.strings.R.string.form_saved)
.checkIsSnackbarWithMessageDisplayed(string.form_saved)

// Check deprecation banner is shown
.assertText(string.edit_finalized_form_warning)
.clickOnString(string.learn_more_button_text)
.also {
intended(
allOf(
hasComponent(WebViewActivity::class.java.name),
hasExtra("url", "https://forum.getodk.org/t/42007")
)
)
}.pressBack(MainMenuPage())

.clickSendFinalizedForm(1)
.clickOnFormToEdit("One Question")
Expand All @@ -64,7 +82,7 @@ class SendFinalizedFormTest {
.answerQuestion("what is your age", "53")
.swipeToEndScreen()
.clickSaveAsDraft()
.checkIsSnackbarWithMessageDisplayed(org.odk.collect.strings.R.string.form_saved_as_draft)
.checkIsSnackbarWithMessageDisplayed(string.form_saved_as_draft)

.clickEditSavedForm(1)
.clickOnForm("One Question")
Expand Down Expand Up @@ -124,7 +142,7 @@ class SendFinalizedFormTest {
.clickViewSentForm(1)
.clickOnForm("One Question")
.assertText("123")
.assertText(org.odk.collect.strings.R.string.exit)
.assertText(string.exit)
}

@Test
Expand Down Expand Up @@ -154,7 +172,7 @@ class SendFinalizedFormTest {
.openProjectSettingsDialog()
.clickSettings()
.clickFormManagement()
.scrollToRecyclerViewItemAndClickText(org.odk.collect.strings.R.string.delete_after_send)
.scrollToRecyclerViewItemAndClickText(string.delete_after_send)
.pressBack(ProjectSettingsPage())
.pressBack(MainMenuPage())
.copyForm("one-question.xml", testDependencies.server.hostName)
Expand Down
7 changes: 5 additions & 2 deletions collect_app/src/main/java/org/odk/collect/android/activities/FormEntryViewModelFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import org.odk.collect.android.projects.CurrentProjectProvider
import org.odk.collect.android.utilities.ApplicationConstants
import org.odk.collect.android.utilities.InstancesRepositoryProvider
import org.odk.collect.android.utilities.MediaUtils
import org.odk.collect.androidshared.data.AppState
import org.odk.collect.async.Scheduler
import org.odk.collect.audiorecorder.recording.AudioRecorder
import org.odk.collect.location.LocationClient
Expand All @@ -46,7 +47,8 @@ class FormEntryViewModelFactory(
private val fusedLocationClient: LocationClient,
private val permissionsProvider: PermissionsProvider,
private val autoSendSettingsProvider: AutoSendSettingsProvider,
private val instancesRepositoryProvider: InstancesRepositoryProvider
private val instancesRepositoryProvider: InstancesRepositoryProvider,
private val appState: AppState
) : AbstractSavedStateViewModelFactory(owner, null) {

override fun <T : ViewModel> create(
Expand Down Expand Up @@ -75,7 +77,8 @@ class FormEntryViewModelFactory(
currentProjectProvider,
formSessionRepository.get(sessionId),
entitiesRepositoryProvider.get(projectId),
instancesRepositoryProvider.get(projectId)
instancesRepositoryProvider.get(projectId),
appState
)
}

Expand Down
4 changes: 3 additions & 1 deletion collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static org.odk.collect.android.utilities.AnimationUtils.areAnimationsEnabled;
import static org.odk.collect.android.utilities.ApplicationConstants.RequestCodes;
import static org.odk.collect.android.utilities.DialogUtils.getDialog;
import static org.odk.collect.androidshared.data.AppStateKt.getState;
import static org.odk.collect.androidshared.ui.DialogFragmentUtils.showIfNotShowing;
import static org.odk.collect.androidshared.ui.ToastUtils.showLongToast;
import static org.odk.collect.androidshared.ui.ToastUtils.showShortToast;
Expand Down Expand Up @@ -430,7 +431,8 @@ public void onCreate(Bundle savedInstanceState) {
fusedLocatonClient,
permissionsProvider,
autoSendSettingsProvider,
instancesRepositoryProvider
instancesRepositoryProvider,
getState(getApplication())
);

this.getSupportFragmentManager().setFragmentFactory(new FragmentFactoryBuilder()
Expand Down
4 changes: 3 additions & 1 deletion collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package org.odk.collect.android.activities;

import static org.odk.collect.android.javarosawrapper.FormIndexUtils.getPreviousLevel;
import static org.odk.collect.androidshared.data.AppStateKt.getState;

import android.content.DialogInterface;
import android.os.Bundle;
Expand Down Expand Up @@ -196,7 +197,8 @@ public void onCreate(Bundle savedInstanceState) {
fusedLocationClient,
permissionsProvider,
autoSendSettingsProvider,
instancesRepositoryProvider
instancesRepositoryProvider,
getState(getApplication())
);

this.getSupportFragmentManager().setFragmentFactory(new FragmentFactoryBuilder()
Expand Down
4 changes: 1 addition & 3 deletions collect_app/src/main/java/org/odk/collect/android/activities/InstanceChooserList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@
import org.odk.collect.forms.instances.Instance;
import org.odk.collect.material.MaterialProgressDialogFragment;
import org.odk.collect.settings.SettingsProvider;
import org.odk.collect.strings.R.string;
import org.odk.collect.strings.R.plurals;
import org.odk.collect.strings.R.string;

import java.util.Arrays;

Expand Down Expand Up @@ -264,8 +264,6 @@ private void logFormEdit(Cursor cursor) {

if (status.equals(Instance.STATUS_INCOMPLETE)) {
AnalyticsUtils.logFormEvent(AnalyticsEvents.EDIT_NON_FINALIZED_FORM, formId, formTitle);
} else if (status.equals(Instance.STATUS_COMPLETE)) {
AnalyticsUtils.logFormEvent(AnalyticsEvents.EDIT_FINALIZED_FORM, formId, formTitle);
Copy link
Member Author

Choose a reason for hiding this comment

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

You're no longer able to edit finalised forms from Drafts so this would never fire!

}
}

Expand Down
13 changes: 12 additions & 1 deletion collect_app/src/main/java/org/odk/collect/android/formentry/saving/FormSaveViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.odk.collect.android.tasks.SaveFormToDisk.SAVED;
import static org.odk.collect.android.tasks.SaveFormToDisk.SAVED_AND_EXIT;
import static org.odk.collect.android.utilities.ApplicationConstants.AppStateKeys.EDITED_FINALIZED_FORM;
import static org.odk.collect.shared.strings.StringUtils.isBlank;

import android.net.Uri;
Expand All @@ -16,6 +17,8 @@

import org.apache.commons.io.IOUtils;
import org.javarosa.form.api.FormEntryController;
import org.odk.collect.analytics.Analytics;
import org.odk.collect.android.analytics.AnalyticsEvents;
import org.odk.collect.android.application.Collect;
import org.odk.collect.android.dao.helpers.InstancesDaoHelper;
import org.odk.collect.android.externaldata.ExternalDataManager;
Expand All @@ -29,6 +32,7 @@
import org.odk.collect.android.utilities.FileUtils;
import org.odk.collect.android.utilities.MediaUtils;
import org.odk.collect.android.utilities.QuestionMediaManager;
import org.odk.collect.androidshared.data.AppState;
import org.odk.collect.androidshared.livedata.LiveDataUtils;
import org.odk.collect.async.Scheduler;
import org.odk.collect.audiorecorder.recording.AudioRecorder;
Expand Down Expand Up @@ -84,8 +88,9 @@ public class FormSaveViewModel extends ViewModel implements MaterialProgressDial
private final EntitiesRepository entitiesRepository;
private final InstancesRepository instancesRepository;
private Instance instance;
private final AppState appState;

public FormSaveViewModel(SavedStateHandle stateHandle, Supplier<Long> clock, FormSaver formSaver, MediaUtils mediaUtils, Scheduler scheduler, AudioRecorder audioRecorder, CurrentProjectProvider currentProjectProvider, LiveData<FormSession> formSession, EntitiesRepository entitiesRepository, InstancesRepository instancesRepository) {
public FormSaveViewModel(SavedStateHandle stateHandle, Supplier<Long> clock, FormSaver formSaver, MediaUtils mediaUtils, Scheduler scheduler, AudioRecorder audioRecorder, CurrentProjectProvider currentProjectProvider, LiveData<FormSession> formSession, EntitiesRepository entitiesRepository, InstancesRepository instancesRepository, AppState appState) {
this.stateHandle = stateHandle;
this.clock = clock;
this.formSaver = formSaver;
Expand All @@ -95,6 +100,7 @@ public FormSaveViewModel(SavedStateHandle stateHandle, Supplier<Long> clock, For
this.currentProjectProvider = currentProjectProvider;
this.entitiesRepository = entitiesRepository;
this.instancesRepository = instancesRepository;
this.appState = appState;

if (stateHandle.get(ORIGINAL_FILES) != null) {
originalFiles = stateHandle.get(ORIGINAL_FILES);
Expand All @@ -110,6 +116,11 @@ public FormSaveViewModel(SavedStateHandle stateHandle, Supplier<Long> clock, For
}

public void saveForm(Uri instanceContentURI, boolean shouldFinalize, String updatedSaveName, boolean viewExiting) {
if (instance != null && instance.getStatus().equals(Instance.STATUS_COMPLETE)) {
appState.set(EDITED_FINALIZED_FORM, true);
Analytics.log(AnalyticsEvents.EDIT_FINALIZED_FORM, "form");
}

if (isSaving() || formController == null) {
return;
}
Expand Down
32 changes: 24 additions & 8 deletions collect_app/src/main/java/org/odk/collect/android/mainmenu/MainMenuActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ import org.odk.collect.android.injection.DaggerUtils
import org.odk.collect.android.projects.ProjectIconView
import org.odk.collect.android.projects.ProjectSettingsDialog
import org.odk.collect.android.utilities.ApplicationConstants
import org.odk.collect.android.utilities.ApplicationConstants.AppStateKeys.EDITED_FINALIZED_FORM
import org.odk.collect.android.utilities.PlayServicesChecker
import org.odk.collect.android.utilities.ThemeUtils
import org.odk.collect.androidshared.data.getState
import org.odk.collect.androidshared.ui.DialogFragmentUtils.showIfNotShowing
import org.odk.collect.androidshared.ui.FragmentFactoryBuilder
import org.odk.collect.androidshared.ui.SnackbarUtils
Expand All @@ -41,6 +43,7 @@ import org.odk.collect.permissions.PermissionsProvider
import org.odk.collect.projects.Project.Saved
import org.odk.collect.settings.SettingsProvider
import org.odk.collect.settings.keys.ProjectKeys
import org.odk.collect.strings.R.string
import org.odk.collect.strings.localization.LocalizedActivity
import javax.inject.Inject

Expand Down Expand Up @@ -130,7 +133,7 @@ class MainMenuActivity : LocalizedActivity() {
currentProjectViewModel.refresh()
mainMenuViewModel.refreshInstances()
setButtonsVisibility()
manageGoogleDriveDeprecationBanner()
setupDeprecationBanner()
}

private fun setButtonsVisibility() {
Expand All @@ -151,7 +154,7 @@ class MainMenuActivity : LocalizedActivity() {
(projectsMenuItem.actionView as ProjectIconView).apply {
project = currentProjectViewModel.currentProject.value
setOnClickListener { onOptionsItemSelected(projectsMenuItem) }
contentDescription = getString(org.odk.collect.strings.R.string.projects)
contentDescription = getString(string.projects)
}
return super.onPrepareOptionsMenu(menu)
}
Expand Down Expand Up @@ -275,7 +278,7 @@ class MainMenuActivity : LocalizedActivity() {
private fun initAppName() {
binding.appName.text = String.format(
"%s %s",
getString(org.odk.collect.strings.R.string.collect_app_name),
getString(string.collect_app_name),
mainMenuViewModel.version
)

Expand All @@ -287,18 +290,31 @@ class MainMenuActivity : LocalizedActivity() {
}
}

private fun manageGoogleDriveDeprecationBanner() {
private fun setupDeprecationBanner() {
val unprotectedSettings = settingsProvider.getUnprotectedSettings()
val protocol = unprotectedSettings.getString(ProjectKeys.KEY_PROTOCOL)
if (ProjectKeys.PROTOCOL_GOOGLE_SHEETS == protocol) {
binding.googleDriveDeprecationBanner.root.visibility = View.VISIBLE
binding.googleDriveDeprecationBanner.learnMoreButton.setOnClickListener {
val usingGoogleDrive = ProjectKeys.PROTOCOL_GOOGLE_SHEETS == protocol
val editedFinalizedForm =
application.getState().get<Boolean>(EDITED_FINALIZED_FORM) ?: false

if (usingGoogleDrive) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we ever need to display both banners at the same time?

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided that we'd just show the Google Drive one if both apply. From the issue:

Google Drive banner should be shown if they're both applicable

binding.deprecationBanner.root.visibility = View.VISIBLE
binding.deprecationBanner.message.setText(string.google_drive_deprecation_message)
binding.deprecationBanner.learnMoreButton.setOnClickListener {
val intent = Intent(this, WebViewActivity::class.java)
intent.putExtra("url", "https://forum.getodk.org/t/40097")
startActivity(intent)
}
} else if (editedFinalizedForm) {
binding.deprecationBanner.root.visibility = View.VISIBLE
binding.deprecationBanner.message.setText(string.edit_finalized_form_warning)
binding.deprecationBanner.learnMoreButton.setOnClickListener {
val intent = Intent(this, WebViewActivity::class.java)
intent.putExtra("url", "https://forum.getodk.org/t/42007")
startActivity(intent)
}
} else {
binding.googleDriveDeprecationBanner.root.visibility = View.GONE
binding.deprecationBanner.root.visibility = View.GONE
}
}

Expand Down
5 changes: 5 additions & 0 deletions collect_app/src/main/java/org/odk/collect/android/utilities/ApplicationConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,9 @@ public abstract static class Namespaces {
public static final String XML_OPENROSA_NAMESPACE = "http://openrosa.org/xforms";
public static final String XML_OPENDATAKIT_NAMESPACE = "http://www.opendatakit.org/xforms";
}

public abstract static class AppStateKeys {

public static final String EDITED_FINALIZED_FORM = "editedFinalizedForm";
Copy link
Member

Choose a reason for hiding this comment

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

One thing to consider: maybe this should be placed in AppState class. I'm not sure if keeping constant used in different parts of the app and different functionalities in one class is still a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm also not sure about ApplicationConstants, but it did seem like the right place for this at the moment. Maybe we should look at splitting it up next time we want to add something?

I don't think that keys for AppState should live in the AppState class as the keys should be owned by the component/module/package using AppState. I guess you can think about it like a Map - it shouldn't need to know anything about the things it stores to do it's job of just storing them.

}
}
16 changes: 8 additions & 8 deletions ...ayout/google_drive_deprecation_banner.xml → ...rc/main/res/layout/deprecation_banner.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:visibility="gone"
app:shapeAppearance="?shapeAppearanceLargeComponent"
app:cardBackgroundColor="?colorSurfaceContainerHighest"
app:shapeAppearance="?shapeAppearanceLargeComponent"
tools:visibility="visible">

<androidx.constraintlayout.widget.ConstraintLayout
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:paddingTop="@dimen/margin_standard"
android:paddingHorizontal="@dimen/margin_standard">
android:paddingHorizontal="@dimen/margin_standard"
android:paddingTop="@dimen/margin_standard">

<ImageView
android:id="@+id/instance_name_info_icon"
android:id="@+id/deprecation_icon"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:src="@drawable/ic_outline_info_24"
Expand All @@ -29,11 +29,11 @@
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginStart="@dimen/margin_standard"
android:text="@string/google_drive_deprecation_message"
android:textAppearance="?textAppearanceBody2"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toEndOf="@id/instance_name_info_icon"
app:layout_constraintTop_toTopOf="@id/instance_name_info_icon" />
app:layout_constraintStart_toEndOf="@id/deprecation_icon"
app:layout_constraintTop_toTopOf="@id/deprecation_icon"
tools:text="@string/google_drive_deprecation_message" />

<com.google.android.material.button.MaterialButton
android:id="@+id/learn_more_button"
Expand All @@ -47,4 +47,4 @@
app:layout_constraintTop_toBottomOf="@+id/message" />
</androidx.constraintlayout.widget.ConstraintLayout>

</com.google.android.material.card.MaterialCardView>
</com.google.android.material.card.MaterialCardView>
6 changes: 3 additions & 3 deletions collect_app/src/main/res/layout/form_entry_end.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ the specific language governing permissions and limitations under the License.
android:padding="@dimen/margin">

<ImageView
android:id="@+id/instance_name_info_icon"
android:id="@+id/deprecation_icon"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:src="@drawable/ic_outline_info_24"
Expand All @@ -70,8 +70,8 @@ the specific language governing permissions and limitations under the License.
android:layout_marginStart="@dimen/margin"
android:textAppearance="?textAppearanceBodyLarge"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toEndOf="@id/instance_name_info_icon"
app:layout_constraintTop_toTopOf="@id/instance_name_info_icon"
app:layout_constraintStart_toEndOf="@id/deprecation_icon"
app:layout_constraintTop_toTopOf="@id/deprecation_icon"
tools:text="@string/form_edits_warning_save_as_draft_and_finalize_enabled"/>
</androidx.constraintlayout.widget.ConstraintLayout>
</com.google.android.material.card.MaterialCardView>
Expand Down
8 changes: 4 additions & 4 deletions collect_app/src/main/res/layout/main_menu.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
<include
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:id="@+id/google_drive_deprecation_banner"
layout="@layout/google_drive_deprecation_banner"
android:id="@+id/deprecation_banner"
layout="@layout/deprecation_banner"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/map_box_initialization_fragment" />
Expand All @@ -49,7 +49,7 @@
app:layout_constraintWidth_max="@dimen/max_content_width"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@id/google_drive_deprecation_banner" />
app:layout_constraintTop_toBottomOf="@id/deprecation_banner" />

<org.odk.collect.android.mainmenu.MainMenuButton
android:id="@+id/review_data"
Expand Down Expand Up @@ -128,4 +128,4 @@

</androidx.constraintlayout.widget.ConstraintLayout>
</ScrollView>
</androidx.constraintlayout.widget.ConstraintLayout>
</androidx.constraintlayout.widget.ConstraintLayout>
Loading