SQ-60/Check remote config for onboarding #256

rock3r · 2017-04-04T08:29:55Z

This PR is HUGE and I'm really sorry about that but I went up and down paths that led me nowhere. I have tidied things up as much as I could but it's still a significant amount of changes, because the original idea turned out to be somewhat flawed. Sorryyyyy

The idea is that now we have a ProximityFeature that checks whether the Bluetooth is available on the device, and if the kill switch has not been pulled. There's a lot of refactoring and cleaning (hence a noisy diff).

Next (and last) step is to understand why the preferences don't look disabled when they are, and possibly use the Feature in the Preconditions to reduce duplication.

…ctive

Still needs cleaning up

fourlastor

Few questions 💃

fourlastor · 2017-04-04T09:14:36Z

app/src/main/java/net/squanchy/navigation/RoutingInjector.java

@@ -21,6 +23,8 @@ public static RoutingComponent obtain(Activity activity) {
                .navigationModule(new NavigationModule())
                .signInModule(new SignInModule())
                .routingModule(new RoutingModule())
+                .proximityFeatureModule(new ProximityFeatureModule())


Why does this have the proximity feature?

Used by the Onboarding class (through the OnboardingModule) to decide whether we can show the Proximity onboarding page (https://github.com/rock3r/squanchy/pull/256/files#diff-e08306406758fca42ffe92c82be63267R35)

fourlastor · 2017-04-04T09:16:14Z

app/src/main/java/net/squanchy/onboarding/location/LocationOnboardingActivity.java

+            }
+
+            @Override
+            public void featureDisabled() {


can we have Timber.e something here? This shouldn't happen I suppose (as in you shouldn't get here to start with if the feature is disabled)

abc912e (same for the SettingsFragment)

fourlastor · 2017-04-04T09:17:39Z

app/src/main/java/net/squanchy/onboarding/location/LocationOnboardingActivity.java

+        };
+    }
+
+    private void showNonFatalProximityError(Snackbar snackbar) {


can we do the same as showFatalProximityError? As in passing only the @StringRes and making the snackbar here.

Ok will have to make it more complicated because some snackbars have actions

Oh. Used to. Failed miserably and now we have two things that show the same message. That is bad. Will fix

Don't really care about that case that was covered by the snackbar actions, so I will simply uniform all the things.

fourlastor · 2017-04-04T09:18:59Z

app/src/main/java/net/squanchy/proximity/BluetoothModule.java

+public class BluetoothModule {
+
+    @Provides
+    BluetoothManager bluetoothManager(Activity activity) {


do you really need an activity? what about a context?

Good luck injecting a Context. Dagger will explode because there's different kinds of Context, Application and Activity, and it doesn't know which one to use. This could, I suppose, use an Application instead. But no, you cannot have a @Provide Context if you need to also distinguish between Activity and Application in other modules (which is our case).

You technically could 😅 using a qualifier :P but ok

fourlastor · 2017-04-04T09:23:57Z

app/src/main/java/net/squanchy/proximity/preconditions/PreconditionsRegistry.java

@@ -15,9 +15,9 @@

    boolean anyUnsatisfied() {
        for (Precondition precondition : preconditions) {
-            boolean canCheck = precondition.performsSynchronousSatisfiedCheck();
+            //boolean canCheckIfSatisfied = precondition.performsSynchronousSatisfiedCheck();


do you need this?

No, will take out

fourlastor · 2017-04-04T09:31:01Z

app/src/main/java/net/squanchy/settings/SettingsFragment.java

+                .addApi(LocationServices.API)
+                .build();
+
+        SettingsFragmentComponent component = SettingsInjector.obtainForFragment(


why is there an obtainForFragment method?

Two different components (Activity and Fragment), one injector

fourlastor · 2017-04-04T09:31:54Z

app/src/main/java/net/squanchy/settings/SettingsFragment.java

+        return new ProximityPreconditions.Callback() {
+            @Override
+            public void notOptedIn() {
+                showProximityEnablingError(Snackbar.make(getViewOrThrow(), R.string.proximity_error_not_opted_in, Snackbar.LENGTH_LONG));


I think you can encapsulate Snackbar.make inside showProximityEnablingError and pass only the message

Done already in 615db96 addressing the equivalent comment in LocationOnboardingActivity. This was once used to offer actions in snackbars, then I took them out and forgot to re-simplify this. Sorry.

fourlastor · 2017-04-04T09:34:51Z

app/src/main/res/values/strings.xml

-  <string name="onboarding_error_location_failed_action">ENABLE MANUALLY</string>
-  <string name="onboarding_error_google_client_connection">Unable to verify location settings for the device. Please try again shortly.</string>
+
+  <string name="proximity_error_not_opted_in">Sorry, unable to opt in because… you’re not opted in? If that makes any sense.</string>


you didn't opt in? It's not like a state in which you are 👅

It's an illegal state, someone started the preconditions satisfaction action (which is meant to be run after opting in) while not opted in :)

Yeah but what I meant is "You're not opted in" isn't correct, the correct form is "You didn't opt in"

fourlastor · 2017-04-04T09:35:24Z

app/src/main/res/values/strings.xml

+
+  <string name="proximity_error_not_opted_in">Sorry, unable to opt in because… you’re not opted in? If that makes any sense.</string>
+  <string name="proximity_error_remote_config_kill_switch">Sorry, the feature has been disabled remotely.</string>
+  <string name="proximity_error_permission_denied">We need the location permission to continue.</string>


the proximity_error_location_denied string is the same

fourlastor · 2017-04-04T09:35:42Z

app/src/main/res/values/strings.xml

+  <string name="proximity_error_location_denied">We need the location permission to continue.</string>
+  <string name="proximity_error_bluetooth_denied">We need the Bluetooth to be active to continue.</string>
+  <string name="proximity_error_location_failed">Unable to verify location settings for the device.</string>
+  <string name="proximity_error_location_failed_action">ENABLE MANUALLY</string>


what is this? XD

Old thing. Will take out.

This reverts commit e67c8d3.

rock3r added 20 commits April 4, 2017 08:00

Preload RemoteConfig on app startup

dedf80f

Create RemoteConfigPrecondition

c21779b

Move navigating to location settings to Preconditions

ba09989

Create distinct components for activity and fragment

78a7bf7

Bypass remoteconfig precondition if contest testing debug option is a…

bcc1078

…ctive

Disable SimplifiableIfStatement inspection as it's just annoying

d9aabd2

[WIP] Integrate preconditions check in settings

e7cf2a8

Integrate preconditions check in settings

4675033

Still needs cleaning up

Reduce amount of state in preconditions, streamline logic

8a77453

Define TaskLauncher interface to run tasks

3c83a37

Define TaskLauncher interface to run tasks

3afb9fd

Only show onboarding when the feature has not been killed

615c168

Extract ProximityFeature

432cc03

Move "feature flag" functionality to its own class

f966a61

Improve settings UX

d2a36f3

Remove performsSynchronousSatisfiedCheck() as it's not needed anymore

7e97033

Add onboarding debugging facility

f91ccee

Undo bad merge on Theme.Squanchy.About

d68dfc5

Consider debug flag when doing onboarding

734eac1

Streamline dependency injection modules

22a95df

rock3r requested a review from fourlastor April 4, 2017 08:29

Move ContestTester to main sourceset

e67c8d3

fourlastor suggested changes Apr 4, 2017

View reviewed changes

rock3r added 7 commits April 4, 2017 12:10

Log fatal errors in the proximity preconditions pipeline callbacks

abc912e

Simplify error snackbar generation, fix copypasta in error strings

615db96

Make the BluetoothModule take an Application instead of Activity

fe13188

Remove commented out code

77ba382

Remove unused string

d89d73f

Rename settings activity injection method

46dbe2b

Fix typo in string

d57cdb9

rock3r added 2 commits April 4, 2017 15:34

Revert "Move ContestTester to main sourceset"

68d831e

This reverts commit e67c8d3.

Ensure all CI builds are clean (just in case)

2c12504

fourlastor approved these changes Apr 4, 2017

View reviewed changes

fourlastor merged commit 56f7cf2 into SQ-60/onboarding_feature Apr 4, 2017

rock3r deleted the SQ-60/Check_RemoteConfig branch April 4, 2017 15:17

rock3r mentioned this pull request Apr 4, 2017

SQ-60/Cleanup #261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQ-60/Check remote config for onboarding #256

SQ-60/Check remote config for onboarding #256

rock3r commented Apr 4, 2017

fourlastor left a comment

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

rock3r Apr 4, 2017

rock3r Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

fourlastor Apr 4, 2017

rock3r Apr 4, 2017

rock3r Apr 4, 2017

SQ-60/Check remote config for onboarding #256

SQ-60/Check remote config for onboarding #256

Conversation

rock3r commented Apr 4, 2017

fourlastor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment