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 a bug causing Updates to get stale #2850

Merged
merged 7 commits into from
Sep 18, 2024
Merged

Fixes a bug causing Updates to get stale #2850

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8a9ab5
log whole exception
shubham1g5 Sep 8, 2024
2987308
An up to date app should be treated as success, logs
shubham1g5 Sep 8, 2024
67adfac
Remove update failure handling from outer wrapper as it gets handled …
shubham1g5 Sep 8, 2024
df2f949
A user session is not required for the app update as long as we have …
shubham1g5 Sep 8, 2024
b166a5a
reorganise a little for readability
shubham1g5 Sep 8, 2024
cfba61e
login should not be necessary to update
shubham1g5 Sep 10, 2024
c253475
We are treating up to date app as success now
shubham1g5 Sep 10, 2024
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
9 changes: 5 additions & 4 deletions app/src/org/commcare/heartbeat/HeartbeatWorker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,14 @@ class HeartbeatWorker(context: Context, workerParams: WorkerParameters):
Result.retry()
}
else -> {
Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS,
"Encountered unexpected exception during heartbeat communications: "
+ e.message + ". Stopping the heartbeat thread.")
Logger.exception(
"Encountered unexpected exception during heartbeat communications, stopping the heartbeat thread.",
e
)
Result.failure()
}
}
}
return Result.success()
}
}
}
9 changes: 4 additions & 5 deletions app/src/org/commcare/update/UpdateHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.commcare.resources.ResourceInstallContext;
import org.commcare.resources.model.InstallCancelled;
import org.commcare.resources.model.InstallCancelledException;
import org.commcare.resources.model.InstallRequestSource;
import org.commcare.resources.model.InvalidResourceException;
import org.commcare.resources.model.Resource;
import org.commcare.resources.model.ResourceTable;
Expand Down Expand Up @@ -84,7 +83,7 @@ public static UpdateHelper getNewInstance(boolean autoUpdate, UpdateProgressList

// Main UpdateHelper function for staging updates
public ResultAndError<AppInstallStatus> update(String profileRef, ResourceInstallContext resourceInstallContext) {
setupUpdate(profileRef);
setupUpdate(profileRef, resourceInstallContext);

try {
return new ResultAndError<>(stageUpdate(profileRef, resourceInstallContext));
Expand Down Expand Up @@ -123,10 +122,10 @@ public ResultAndError<AppInstallStatus> update(String profileRef, ResourceInstal
}
}

private void setupUpdate(String profileRef) {
private void setupUpdate(String profileRef, ResourceInstallContext resourceInstallContext) {
ResourceInstallUtils.recordUpdateAttemptTime(mApp);
Logger.log(LogTypes.TYPE_RESOURCES,
"Beginning install attempt for profile " + profileRef);
"Beginning install attempt as " + resourceInstallContext.getInstallRequestSource() + " for profile " + profileRef);

if (isAutoUpdate) {
ResourceInstallUtils.recordAutoUpdateStart(mApp);
Expand All @@ -150,7 +149,7 @@ private AppInstallStatus stageUpdate(String profileRef, ResourceInstallContext r

AppInstallStatus result = mResourceManager.checkAndPrepareUpgradeResources(profileRefWithParams, mAuthority, resourceInstallContext);

if (result == AppInstallStatus.UpdateStaged) {
if (result == AppInstallStatus.UpdateStaged || result == AppInstallStatus.UpToDate) {
RequestStats.markSuccess(resourceInstallContext.getInstallRequestSource());
}

Expand Down
29 changes: 15 additions & 14 deletions app/src/org/commcare/update/UpdateWorker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,31 +44,31 @@ class UpdateWorker(appContext: Context, workerParams: WorkerParameters)
when {
exception is CancellationException -> handleUpdateResult(ResultAndError(AppInstallStatus.Cancelled))
exception != null -> {
Logger.exception("Unknown error while app update", exception);
handleUpdateResult(ResultAndError(AppInstallStatus.UnknownFailure))
Logger.exception("Unknown error while app update", exception)
Result.failure()
}
}
}

job.await()
}

}

private fun doUpdateWork(): Result {
val updateResult: ResultAndError<AppInstallStatus>

// skip if - An update task is already running | no app is seated | user session is not active
if (UpdateTask.getRunningInstance() == null &&
CommCareApplication.instance().currentApp != null &&
CommCareApplication.instance().session.isActive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just a clarification, in the previous version, in case there was no active user session the result would be success, so what was causing the errors when there was no active session?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the call to CommCareApplication.instance().session.isActive would itself error out with a SessionUnavailableException

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh yes, I see it now


updateHelper.startPinnedNotification(CommCareApplication.instance())
updateResult = updateHelper.update(ResourceInstallUtils.getDefaultProfileRef(),
ResourceInstallContext(InstallRequestSource.BACKGROUND_UPDATE))
} else {
if (UpdateTask.getRunningInstance() != null) {
// there is already an update running, lets just skip this run
return Result.success()
}

if (CommCareApplication.instance().currentApp == null) {
// we need a seated app to update
return Result.failure()
}

updateHelper.startPinnedNotification(CommCareApplication.instance())
val updateResult: ResultAndError<AppInstallStatus> = updateHelper.update(
ResourceInstallUtils.getDefaultProfileRef(),
ResourceInstallContext(InstallRequestSource.BACKGROUND_UPDATE))
return handleUpdateResult(updateResult)
}

Expand All @@ -82,6 +82,7 @@ class UpdateWorker(appContext: Context, workerParams: WorkerParameters)

return when {
updateResult.data == AppInstallStatus.UpdateStaged -> Result.success()
updateResult.data == AppInstallStatus.UpToDate -> Result.success()
updateResult.data.shouldRetryUpdate() -> Result.retry()
else -> Result.failure()
}
Expand Down
4 changes: 2 additions & 2 deletions app/unit-tests/src/org/commcare/android/tests/application/AppUpdateTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class AppUpdateTest {
UpdateUtils.installUpdate(profileRef,
AppInstallStatus.UpToDate,
AppInstallStatus.UnknownFailure)
checkUpdateComplete(6, true, false)
checkUpdateComplete(6, true, true)
}

@Test
Expand Down Expand Up @@ -191,4 +191,4 @@ class AppUpdateTest {
private val TAG = AppUpdateTest::class.java.simpleName
private const val REF_BASE_DIR = "jr://resource/commcare-apps/update_tests/"
}
}
}
7 changes: 3 additions & 4 deletions app/unit-tests/src/org/commcare/update/UpdateWorkerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ class UpdateWorkerTest {
@Before
fun setUp() {
context = ApplicationProvider.getApplicationContext()
TestAppInstaller.installAppAndLogin(
UpdateUtils.buildResourceRef(REF_BASE_DIR, "base_app", "profile.ccpr"),
"test", "123")
TestAppInstaller.installApp(
UpdateUtils.buildResourceRef(REF_BASE_DIR, "base_app", "profile.ccpr"))
}

@Test
Expand Down Expand Up @@ -114,4 +113,4 @@ class UpdateWorkerTest {
.putString(PREFS_APP_SERVER_KEY, profileRef)
.apply()
}
}
}