From 81fcd44b66a83b3889bcd75e3cd92f76d457b6f5 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 6 Mar 2014 08:05:51 +1100 Subject: [PATCH 1/2] Fixed update notification count The update notification was not taking ignored apps into account. This is the first manifestation of a class of bug I feared whereby the properties of an App object are not initialized, but no error is thrown. It occured because we were iterating over apps that were created from the index file, rather than our database. Hence, they had no knowledge about whether they should be ignored or not. Also took the chance to perform a minor refactor of UpdateService class. The onHandleIntent method was getting huge, so I extracted two methods: verifyIsTimeForScheduledRun() and performUpdateNotification(), as well as removing the unused "success" flag. The two methods should theoretically make the class more testable later, as we can test the scheduled run code, and the update notification code in separate tests, but we'll wait and see if that eventuates. --- src/org/fdroid/fdroid/UpdateService.java | 147 +++++++++++--------- src/org/fdroid/fdroid/data/AppProvider.java | 28 +++- 2 files changed, 110 insertions(+), 65 deletions(-) diff --git a/src/org/fdroid/fdroid/UpdateService.java b/src/org/fdroid/fdroid/UpdateService.java index 5ff446905..f22b93281 100644 --- a/src/org/fdroid/fdroid/UpdateService.java +++ b/src/org/fdroid/fdroid/UpdateService.java @@ -201,6 +201,45 @@ public class UpdateService extends IntentService implements ProgressListener { return receiver == null; } + /** + * Check whether it is time to run the scheduled update. + * We don't want to run if: + * - The time between scheduled runs is set to zero (though don't know + * when that would occur) + * - Last update was too recent + * - Not on wifi, but the property for "Only auto update on wifi" is set. + * @return True if we are due for a scheduled update. + */ + private boolean verifyIsTimeForScheduledRun() { + SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(getBaseContext()); + long lastUpdate = prefs.getLong(Preferences.PREF_UPD_LAST, 0); + String sint = prefs.getString(Preferences.PREF_UPD_INTERVAL, "0"); + int interval = Integer.parseInt(sint); + if (interval == 0) { + Log.d("FDroid", "Skipping update - disabled"); + return false; + } + long elapsed = System.currentTimeMillis() - lastUpdate; + if (elapsed < interval * 60 * 60 * 1000) { + Log.d("FDroid", "Skipping update - done " + elapsed + + "ms ago, interval is " + interval + " hours"); + return false; + } + + // If we are to update the repos only on wifi, make sure that + // connection is active + if (prefs.getBoolean(Preferences.PREF_UPD_WIFI_ONLY, false)) { + ConnectivityManager conMan = (ConnectivityManager) getSystemService(Context.CONNECTIVITY_SERVICE); + NetworkInfo.State wifi = conMan.getNetworkInfo(1).getState(); + if (wifi != NetworkInfo.State.CONNECTED && + wifi != NetworkInfo.State.CONNECTING) { + Log.d("FDroid", "Skipping update - wifi not available"); + return false; + } + } + return true; + } + @Override protected void onHandleIntent(Intent intent) { @@ -210,39 +249,13 @@ public class UpdateService extends IntentService implements ProgressListener { long startTime = System.currentTimeMillis(); String errmsg = ""; try { - - SharedPreferences prefs = PreferenceManager - .getDefaultSharedPreferences(getBaseContext()); + SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(getBaseContext()); // See if it's time to actually do anything yet... - if (isScheduledRun()) { - long lastUpdate = prefs.getLong(Preferences.PREF_UPD_LAST, 0); - String sint = prefs.getString(Preferences.PREF_UPD_INTERVAL, "0"); - int interval = Integer.parseInt(sint); - if (interval == 0) { - Log.d("FDroid", "Skipping update - disabled"); - return; - } - long elapsed = System.currentTimeMillis() - lastUpdate; - if (elapsed < interval * 60 * 60 * 1000) { - Log.d("FDroid", "Skipping update - done " + elapsed - + "ms ago, interval is " + interval + " hours"); - return; - } - - // If we are to update the repos only on wifi, make sure that - // connection is active - if (prefs.getBoolean(Preferences.PREF_UPD_WIFI_ONLY, false)) { - ConnectivityManager conMan = (ConnectivityManager) getSystemService(Context.CONNECTIVITY_SERVICE); - NetworkInfo.State wifi = conMan.getNetworkInfo(1).getState(); - if (wifi != NetworkInfo.State.CONNECTED && - wifi != NetworkInfo.State.CONNECTING) { - Log.d("FDroid", "Skipping update - wifi not available"); - return; - } - } - } else { + if (!isScheduledRun()) { Log.d("FDroid", "Unscheduled (manually requested) update"); + } else if (!verifyIsTimeForScheduledRun()) { + return; } // Grab some preliminary information, then we can release the @@ -255,7 +268,6 @@ public class UpdateService extends IntentService implements ProgressListener { List unchangedRepos = new ArrayList(); List updatedRepos = new ArrayList(); List disabledRepos = new ArrayList(); - boolean success = true; boolean changes = false; for (Repo repo : repos) { @@ -289,14 +301,10 @@ public class UpdateService extends IntentService implements ProgressListener { } } - if (!changes && success) { - Log.d("FDroid", - "Not checking app details or compatibility, " + - "because all repos were up to date."); - } else if (changes && success) { - - sendStatus(STATUS_INFO, - getString(R.string.status_checking_compatibility)); + if (!changes) { + Log.d("FDroid", "Not checking app details or compatibility, ecause all repos were up to date."); + } else { + sendStatus(STATUS_INFO, getString(R.string.status_checking_compatibility)); List listOfAppsToUpdate = new ArrayList(); listOfAppsToUpdate.addAll(appsToUpdate.values()); @@ -312,34 +320,19 @@ public class UpdateService extends IntentService implements ProgressListener { removeApksNoLongerInRepo(listOfAppsToUpdate, updatedRepos); removeAppsWithoutApks(); notifyContentProviders(); - } - if (success && changes && prefs.getBoolean(Preferences.PREF_UPD_NOTIFY, false)) { - int updateCount = 0; - for (App app : appsToUpdate.values()) { - if (app.canAndWantToUpdate(this)) { - updateCount++; - } - } - - if (updateCount > 0) { - showAppUpdatesNotification(updateCount); + if (prefs.getBoolean(Preferences.PREF_UPD_NOTIFY, false)) { + performUpdateNotification(appsToUpdate.values()); } } - if (!success) { - if (errmsg.length() == 0) - errmsg = "Unknown error"; - sendStatus(STATUS_ERROR, errmsg); + Editor e = prefs.edit(); + e.putLong(Preferences.PREF_UPD_LAST, System.currentTimeMillis()); + e.commit(); + if (changes) { + sendStatus(STATUS_COMPLETE_WITH_CHANGES); } else { - Editor e = prefs.edit(); - e.putLong(Preferences.PREF_UPD_LAST, System.currentTimeMillis()); - e.commit(); - if (changes) { - sendStatus(STATUS_COMPLETE_WITH_CHANGES); - } else { - sendStatus(STATUS_COMPLETE_AND_SAME); - } + sendStatus(STATUS_COMPLETE_AND_SAME); } } catch (Exception e) { @@ -464,7 +457,35 @@ public class UpdateService extends IntentService implements ProgressListener { } } - private void showAppUpdatesNotification(int updates) throws Exception { + private void performUpdateNotification(Collection apps) { + int updateCount = 0; + + // This may be somewhat strange, because we usually would just trust + // App.canAndWantToUpdate(). The only problem is that the "appsToUpdate" + // list only contains data from the repo index, not our database. + // As such, it doesn't know if we want to ignore the apps or not. For that, we + // need to query the database manually and identify those which are to be ignored. + String[] projection = { AppProvider.DataColumns.APP_ID }; + List appsToIgnore = AppProvider.Helper.findIgnored(this, projection); + for (App app : apps) { + boolean ignored = false; + for(App appIgnored : appsToIgnore) { + if (appIgnored.id.equals(app.id)) { + ignored = true; + break; + } + } + if (!ignored && app.hasUpdates(this)) { + updateCount++; + } + } + + if (updateCount > 0) { + showAppUpdatesNotification(updateCount); + } + } + + private void showAppUpdatesNotification(int updates) { Log.d("FDroid", "Notifying " + updates + " updates."); NotificationCompat.Builder builder = new NotificationCompat.Builder(this) diff --git a/src/org/fdroid/fdroid/data/AppProvider.java b/src/org/fdroid/fdroid/data/AppProvider.java index 9475cfb62..76d0121fb 100644 --- a/src/org/fdroid/fdroid/data/AppProvider.java +++ b/src/org/fdroid/fdroid/data/AppProvider.java @@ -7,6 +7,7 @@ import android.net.Uri; import android.util.Log; import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.R; +import org.fdroid.fdroid.UpdateService; import org.fdroid.fdroid.Utils; import java.util.*; @@ -32,6 +33,12 @@ public class AppProvider extends FDroidProvider { return cursorToList(cursor); } + public static List findIgnored(Context context, String[] projection) { + Uri uri = AppProvider.getIgnoredUri(); + Cursor cursor = context.getContentResolver().query(uri, projection, null, null, null); + return cursorToList(cursor); + } + private static List cursorToList(Cursor cursor) { List apps = new ArrayList(); if (cursor != null) { @@ -227,6 +234,7 @@ public class AppProvider extends FDroidProvider { private static final String PATH_RECENTLY_UPDATED = "recentlyUpdated"; private static final String PATH_NEWLY_ADDED = "newlyAdded"; private static final String PATH_CATEGORY = "category"; + private static final String PATH_IGNORED = "ignored"; private static final int CAN_UPDATE = CODE_SINGLE + 1; private static final int INSTALLED = CAN_UPDATE + 1; @@ -236,9 +244,11 @@ public class AppProvider extends FDroidProvider { private static final int RECENTLY_UPDATED = APPS + 1; private static final int NEWLY_ADDED = RECENTLY_UPDATED + 1; private static final int CATEGORY = NEWLY_ADDED + 1; + private static final int IGNORED = CATEGORY + 1; static { matcher.addURI(getAuthority(), null, CODE_LIST); + matcher.addURI(getAuthority(), PATH_IGNORED, IGNORED); matcher.addURI(getAuthority(), PATH_RECENTLY_UPDATED, RECENTLY_UPDATED); matcher.addURI(getAuthority(), PATH_NEWLY_ADDED, NEWLY_ADDED); matcher.addURI(getAuthority(), PATH_CATEGORY + "/*", CATEGORY); @@ -262,6 +272,10 @@ public class AppProvider extends FDroidProvider { return Uri.withAppendedPath(getContentUri(), PATH_NEWLY_ADDED); } + public static Uri getIgnoredUri() { + return Uri.withAppendedPath(getContentUri(), PATH_IGNORED); + } + public static Uri getCategoryUri(String category) { return getContentUri().buildUpon() .appendPath(PATH_CATEGORY) @@ -388,6 +402,12 @@ public class AppProvider extends FDroidProvider { return new QuerySelection(selection, args); } + private QuerySelection queryIgnored() { + String selection = "fdroid_app.ignoreAllUpdates = 1 OR " + + "fdroid_app.ignoreThisUpdate >= fdroid_app.suggestedVercode"; + return new QuerySelection(selection); + } + private QuerySelection queryNewlyAdded() { String selection = "fdroid_app.added > ?"; String[] args = { Utils.DATE_FORMAT.format(Preferences.get().calcMaxHistory()) }; @@ -459,6 +479,10 @@ public class AppProvider extends FDroidProvider { query = query.add(queryApps(uri.getLastPathSegment())); break; + case IGNORED: + query = query.add(queryIgnored()); + break; + case CATEGORY: query = query.add(queryCategory(uri.getLastPathSegment())); break; @@ -503,7 +527,7 @@ public class AppProvider extends FDroidProvider { break; default: - throw new UnsupportedOperationException("Can't delete yet"); + throw new UnsupportedOperationException("Delete not supported for " + uri + "."); } @@ -531,7 +555,7 @@ public class AppProvider extends FDroidProvider { break; default: - throw new UnsupportedOperationException("Update not supported for '" + uri + "'."); + throw new UnsupportedOperationException("Update not supported for " + uri + "."); } int count = write().update(getTableName(), values, query.getSelection(), query.getArgs()); From 9fd8da42a1f17695cd17058f71d2a0800f8e43e2 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 6 Mar 2014 15:59:20 +1100 Subject: [PATCH 2/2] Adding test coverage for "AppProvider.Helper.findIgnored()" Also added tests for canAndWantToUpdate() while I was at it. --- src/org/fdroid/fdroid/data/AppProvider.java | 9 -- .../org/fdroid/fdroid/AppProviderTest.java | 110 ++++++++++++++++++ test/src/org/fdroid/fdroid/TestUtils.java | 2 +- 3 files changed, 111 insertions(+), 10 deletions(-) diff --git a/src/org/fdroid/fdroid/data/AppProvider.java b/src/org/fdroid/fdroid/data/AppProvider.java index 76d0121fb..463b57317 100644 --- a/src/org/fdroid/fdroid/data/AppProvider.java +++ b/src/org/fdroid/fdroid/data/AppProvider.java @@ -137,23 +137,14 @@ public class AppProvider extends FDroidProvider { public static final String SUGGESTED_VERSION_CODE = "suggestedVercode"; public static final String UPSTREAM_VERSION = "upstreamVersion"; public static final String UPSTREAM_VERSION_CODE = "upstreamVercode"; - public static final String CURRENT_APK = null; public static final String ADDED = "added"; public static final String LAST_UPDATED = "lastUpdated"; - public static final String INSTALLED_VERSION = null; - public static final String INSTALLED_VERCODE = null; - public static final String USER_INSTALLED = null; public static final String CATEGORIES = "categories"; public static final String ANTI_FEATURES = "antiFeatures"; public static final String REQUIREMENTS = "requirements"; - public static final String FILTERED = null; - public static final String HAS_UPDATES = null; - public static final String TO_UPDATE = null; public static final String IGNORE_ALLUPDATES = "ignoreAllUpdates"; public static final String IGNORE_THISUPDATE = "ignoreThisUpdate"; public static final String ICON_URL = "iconUrl"; - public static final String UPDATED = null; - public static final String APKS = null; public interface SuggestedApk { public static final String VERSION = "suggestedApkVersion"; diff --git a/test/src/org/fdroid/fdroid/AppProviderTest.java b/test/src/org/fdroid/fdroid/AppProviderTest.java index 2768e858b..80e19bd72 100644 --- a/test/src/org/fdroid/fdroid/AppProviderTest.java +++ b/test/src/org/fdroid/fdroid/AppProviderTest.java @@ -1,9 +1,11 @@ package org.fdroid.fdroid; +import android.content.ContentResolver; import android.content.ContentValues; import android.database.Cursor; import mock.MockCategoryResources; +import mock.MockContextSwappableComponents; import mock.MockInstallablePackageManager; import org.fdroid.fdroid.data.ApkProvider; @@ -33,6 +35,10 @@ public class AppProviderTest extends FDroidProviderTest { }; } + public void testCantFindApp() { + assertNull(AppProvider.Helper.findById(getMockContentResolver(), "com.example.doesnt-exist")); + } + public void testUris() { assertInvalidUri(AppProvider.getAuthority()); assertInvalidUri(ApkProvider.getContentUri()); @@ -65,6 +71,110 @@ public class AppProviderTest extends FDroidProviderTest { } } + private void insertAndInstallApp( + MockInstallablePackageManager packageManager, + String id, int installedVercode, int suggestedVercode, + boolean ignoreAll, int ignoreVercode) { + ContentValues values = new ContentValues(3); + values.put(AppProvider.DataColumns.SUGGESTED_VERSION_CODE, suggestedVercode); + values.put(AppProvider.DataColumns.IGNORE_ALLUPDATES, ignoreAll); + values.put(AppProvider.DataColumns.IGNORE_THISUPDATE, ignoreVercode); + insertApp(id, "App: " + id, values); + + packageManager.install(id, installedVercode, "v" + installedVercode); + } + + public void testCanUpdate() { + + MockContextSwappableComponents c = getSwappableContext(); + + MockInstallablePackageManager pm = new MockInstallablePackageManager(); + c.setPackageManager(pm); + + insertApp("not installed", "not installed"); + insertAndInstallApp(pm, "installed, only one version available", 1, 1, false, 0); + insertAndInstallApp(pm, "installed, already latest, no ignore", 10, 10, false, 0); + insertAndInstallApp(pm, "installed, already latest, ignore all", 10, 10, true, 0); + insertAndInstallApp(pm, "installed, already latest, ignore latest", 10, 10, false, 10); + insertAndInstallApp(pm, "installed, already latest, ignore old", 10, 10, false, 5); + insertAndInstallApp(pm, "installed, old version, no ignore", 5, 10, false, 0); + insertAndInstallApp(pm, "installed, old version, ignore all", 5, 10, true, 0); + insertAndInstallApp(pm, "installed, old version, ignore latest", 5, 10, false, 10); + insertAndInstallApp(pm, "installed, old version, ignore newer, but not latest", 5, 10, false, 8); + + ContentResolver r = getMockContentResolver(); + + // Can't "update", although can "install"... + App notInstalled = AppProvider.Helper.findById(r, "not installed"); + assertFalse(notInstalled.canAndWantToUpdate(c)); + + App installedOnlyOneVersionAvailable = AppProvider.Helper.findById(r, "installed, only one version available"); + App installedAlreadyLatestNoIgnore = AppProvider.Helper.findById(r, "installed, already latest, no ignore"); + App installedAlreadyLatestIgnoreAll = AppProvider.Helper.findById(r, "installed, already latest, ignore all"); + App installedAlreadyLatestIgnoreLatest = AppProvider.Helper.findById(r, "installed, already latest, ignore latest"); + App installedAlreadyLatestIgnoreOld = AppProvider.Helper.findById(r, "installed, already latest, ignore old"); + + assertFalse(installedOnlyOneVersionAvailable.canAndWantToUpdate(c)); + assertFalse(installedAlreadyLatestNoIgnore.canAndWantToUpdate(c)); + assertFalse(installedAlreadyLatestIgnoreAll.canAndWantToUpdate(c)); + assertFalse(installedAlreadyLatestIgnoreLatest.canAndWantToUpdate(c)); + assertFalse(installedAlreadyLatestIgnoreOld.canAndWantToUpdate(c)); + + App installedOldNoIgnore = AppProvider.Helper.findById(r, "installed, old version, no ignore"); + App installedOldIgnoreAll = AppProvider.Helper.findById(r, "installed, old version, ignore all"); + App installedOldIgnoreLatest = AppProvider.Helper.findById(r, "installed, old version, ignore latest"); + App installedOldIgnoreNewerNotLatest = AppProvider.Helper.findById(r, "installed, old version, ignore newer, but not latest"); + + assertTrue(installedOldNoIgnore.canAndWantToUpdate(c)); + assertFalse(installedOldIgnoreAll.canAndWantToUpdate(c)); + assertFalse(installedOldIgnoreLatest.canAndWantToUpdate(c)); + assertTrue(installedOldIgnoreNewerNotLatest.canAndWantToUpdate(c)); + } + + public void testIgnored() { + + MockInstallablePackageManager pm = new MockInstallablePackageManager(); + getSwappableContext().setPackageManager(pm); + + insertApp("not installed", "not installed"); + insertAndInstallApp(pm, "installed, only one version available", 1, 1, false, 0); + insertAndInstallApp(pm, "installed, already latest, no ignore", 10, 10, false, 0); + insertAndInstallApp(pm, "installed, already latest, ignore all", 10, 10, true, 0); + insertAndInstallApp(pm, "installed, already latest, ignore latest", 10, 10, false, 10); + insertAndInstallApp(pm, "installed, already latest, ignore old", 10, 10, false, 5); + insertAndInstallApp(pm, "installed, old version, no ignore", 5, 10, false, 0); + insertAndInstallApp(pm, "installed, old version, ignore all", 5, 10, true, 0); + insertAndInstallApp(pm, "installed, old version, ignore latest", 5, 10, false, 10); + insertAndInstallApp(pm, "installed, old version, ignore newer, but not latest", 5, 10, false, 8); + + assertResultCount(10, AppProvider.getContentUri()); + + String[] projection = { AppProvider.DataColumns.APP_ID }; + List ignoredApps = AppProvider.Helper.findIgnored(getMockContext(), projection); + + String[] expectedIgnored = { + "installed, already latest, ignore all", + "installed, already latest, ignore latest", + // NOT "installed, already latest, ignore old" - because it + // is should only ignore if "ignored version" is >= suggested + + "installed, old version, ignore all", + "installed, old version, ignore latest" + // NOT "installed, old version, ignore newer, but not latest" + // for the same reason as above. + }; + + assertContainsOnlyIds(ignoredApps, expectedIgnored); + } + + private void assertContainsOnlyIds(List actualApps, String[] expectedIds) { + List actualIds = new ArrayList(actualApps.size()); + for (App app : actualApps) { + actualIds.add(app.id); + } + TestUtils.assertContainsOnly(actualIds, expectedIds); + } + public void testInstalled() { Utils.clearInstalledApksCache(); diff --git a/test/src/org/fdroid/fdroid/TestUtils.java b/test/src/org/fdroid/fdroid/TestUtils.java index 6d9bbaa2a..1afc0454c 100644 --- a/test/src/org/fdroid/fdroid/TestUtils.java +++ b/test/src/org/fdroid/fdroid/TestUtils.java @@ -24,7 +24,7 @@ public class TestUtils { if (i > 0) { string += ", "; } - string += list.get(i); + string += "'" + list.get(i) + "'"; } string += "]"; return string;