Connect #2695

OrangeAndGreen · 2023-08-24T17:34:02Z

Summary

Added Android Navigation component (and SafeArgs plugin) to the project.
Implemented Connect navigation graph.
Implemented 5 UI screens as fragments and built/populated the UIs.

Product Description

When user unlocks ConnectID on login page, button saying "Go to Connect Menu" now appears.
When clicked, the user sees the Jobs List fragment, and can begin navigating through screens with mock data.

Safety Assurance

[x ] If the PR is high risk, "High Risk" label is set
[x ] I have confidence that this PR will not introduce a regression for the reasons below
[x ] Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Automated test coverage

No tests yet

shubham1g5

The drawable added in the PR should be added according to different Android screen densities - ldpi, hdpi, xhdpi, xxhdpi, xxxhdpi . You should be able to ask the designer for these.

shubham1g5 · 2023-08-29T17:59:20Z

app/res/values/strings.xml

@@ -606,6 +606,35 @@
    <string name="connect_pictures_skip" cc:translatable="true">Skip</string>
    <string name="connect_pictures_continue" cc:translatable="true">Continue</string>

+    <string name="connect_title" cc:translatable="true">Connect</string>


Actually I think all connect strings should have cc:translatable not set. cc:translatable is meant for CC app's translations which are contained in the app ccz bundle and can be different for each apps. But Connect translations are independent of CC app and will be standard across different CC apps.

shubham1g5 · 2023-08-29T18:16:14Z

app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java

+        View view = inflater.inflate(R.layout.fragment_connect_delivery_details, container, false);
+
+        TextView textView = view.findViewById(R.id.connect_delivery_title);
+        textView.setText(getString(R.string.connect_delivery_review_title));


you can set static strings in the xml itself.

shubham1g5 · 2023-08-29T18:23:59Z

app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java

+    public View onCreateView(LayoutInflater inflater, ViewGroup container,
+                             Bundle savedInstanceState) {
+        ConnectJob job = ConnectJobIntroFragmentArgs.fromBundle(getArguments()).getJob();
+        getActivity().setTitle(job.getTitle());


can we use a Android view model to bind the UI here instead ? That will take care of things like restoring data state on configuration changes.

shubham1g5 · 2023-08-29T18:46:10Z

app/src/org/commcare/fragments/connect/ConnectJobsMyListFragment.java

+        return view;
+    }
+
+    private static class MyJobsAdapter extends RecyclerView.Adapter<RecyclerView.ViewHolder> {


This should probably be in a separate file as JobsAdapter which can be used both for all jobs and my jobs tabs.

shubham1g5 · 2023-09-15T19:20:23Z

@OrangeAndGreen Wants to flag that this PR has taken the same course as earlier Connect PR and is becoming very big to manage. Not asking to change anything with the current PR but we must shift our approach on future connect work to do smaller PRs with smaller commits and try to close existing PRs proactively by requesting reviews on them and addressing feedback before taking on new work. As an example, This Android 13 PR demonstrates how to break changes in individual commits for them to be easily reviewed by commit.

shubham1g5 · 2023-09-24T17:47:53Z

app/src/org/commcare/android/database/connect/models/ConnectAppInfo.java

+import org.json.JSONObject;
+
+@Table(ConnectAppInfo.STORAGE_KEY)
+public class ConnectAppInfo extends Persisted {


nit: can we just call it ConnectApp (Similar to ConnectJob) ? We can also do ConnectAppRecord and ConnectJobRecord looking at other similar meta data classes in code base.

54feb9e (and oops, also 5052252)

shubham1g5 · 2023-09-24T17:48:46Z

app/src/org/commcare/android/database/connect/models/ConnectAppInfo.java

+    public String description;
+    @Persisting(7)
+    @MetaField(META_ORGANIZATION)
+    public String organization;


curious how is organization different from domain ?

I think the idea here is to get a displayable name for the organization that's providing the opportunity. In practice what I'm seeing right now is that the organization I get from the server is my username with Connect (i.e. the username of the app designer). That's probably just a placeholder or default value of some sort, but it's different than the domain. It's also worth nothing that the domain for the learn and deliver apps does not have to be the same.

shubham1g5 · 2023-09-24T17:50:43Z

app/src/org/commcare/android/database/connect/models/ConnectAppInfo.java

+        app.domain = json.has(META_DOMAIN) ? json.getString(META_DOMAIN) : null;
+        app.appId = json.has(META_APP_ID) ? json.getString(META_APP_ID) : null;
+        app.name = json.has(META_NAME) ? json.getString(META_NAME) : null;
+        app.description = json.has(META_DESCRIPTION) ? json.getString(META_DESCRIPTION) : null;
+        app.organization = json.has(META_ORGANIZATION) ? json.getString(META_ORGANIZATION) : null;


Do we know what fields here are optional and what not ? I would like us to throw an exception from here for fields like domain and appID which I would suppose are mandatory fields. We should also appropriately mark nullable fields with @nullable and @Persisting(nullable = true) annotations in their declarations.

shubham1g5 · 2023-09-24T17:53:32Z

app/src/org/commcare/android/database/connect/models/ConnectJob.java

+        this.deliveries = deliveries;
+    }
+
+    public static ConnectJob fromJson(JSONObject json) throws JSONException, ParseException {


same comment as ConnectAppInfo that we should do data validation here for mandatory properties.

shubham1g5 · 2023-09-24T17:58:14Z

app/src/org/commcare/activities/connect/ConnectIdDatabaseHelper.java

+        return getJobs(context, ConnectJob.STATUS_LEARNING);
+    }
+
+    public static List<ConnectJob> getClaimdeJobs(Context context) {


nit: spelling

Fixed in f6c3b16

shubham1g5 · 2023-09-24T17:59:09Z

app/src/org/commcare/activities/connect/ConnectIdDatabaseHelper.java

@@ -136,4 +140,74 @@ public static void setRegistrationPhase(Context context, ConnectIdTask phase) {
            storeUser(context, user);
        }
    }
+
+    public static void storeAvailableJobs(Context context, List<ConnectJob> jobs) {


nit: should we rename the class to ConnectDatabaseHelper since we are using it for all connect db helpers

Agreed. I'll also rename ConnectIdNetworkHelper to drop the Id since it's also being used more widely now. Is there a better place for these classes to live instead of in the activities folder?

Adding ConnectIdSsoHelper to this list also

shubham1g5 · 2023-09-24T18:02:33Z

app/src/org/commcare/activities/connect/ConnectIdDatabaseHelper.java

+            }
+
+            if(!stillExists) {
+                getConnectStorage(context, ConnectJob.class).remove(existing.getID());


how about using removeAll instead with a list of ids ? (bulk db operation vs multiple singular ones)

shubham1g5 · 2023-09-24T18:05:00Z

app/src/org/commcare/activities/connect/ConnectIdDatabaseHelper.java

+            for (ConnectJob existingJob : existingList) {
+                if(newJob.getID() == existingJob.getID()) {
+                    //To update, set ID for the new job from the existing
+                    newJob.setID(existingJob.getID());
+                    break;
+                }
+            }


can we merge this with the loop to find non-existing jobs ? They are both nXn loops so I will prefer to only do these iterations once if possible.

Improved in f6c3b16 so that the first nXn loop sets the ID for jobs found in the existing list, reducing the second nXn loop to only n.

shubham1g5 · 2023-09-24T18:12:42Z

app/src/org/commcare/activities/connect/ConnectIdNetworkHelper.java

+        HashMap<String, String> headers = new HashMap<>();
+        RequestBody requestBody;
+
+        //TODO: Figure out how to send GET request the right way


can we define a Retrofit Interfaces for standard connect Urls in a separate class like - ConnectRequestEndpoints . You can define method like - public Response<ResponseBody> getJobs(String uri, Multimap<String, String> httpParams, Map<String, String> httpHeaders) throws IOException { in there and define corresponding job url with annotations right in that interface. CommCare Urls are not static (they change for every domain) which is why we don't adpot the similar strategy for CC but we don't have that constraint with connect urls.

shubham1g5 · 2023-09-24T18:29:29Z

app/src/org/commcare/activities/connect/ConnectIdSsoHelper.java

@@ -70,7 +70,24 @@ public static AuthInfo.TokenAuth acquireSsoTokenSync(Context context) {
        return hqTokenAuth;
    }

-    private static AuthInfo.TokenAuth retrieveConnectToken(Context context) {
+    public static void retrieveConnectTokenAsync(Context context, TokenCallback callback) {


looks like what's happening here is - Fragment(UI) -> getConnectOpportunities(UI) -> retrieveConnectTokenAsync(UI) -> creates a new thread(t) -> callback(t) -> getInternal()(t) -> triggers AsyncTask(t) . My question is does not an async task needs to get triggered from UI thread ?

I changed the code to use an AsyncTask instead, ae8132a

OrangeAndGreen · 2023-09-29T20:54:55Z

@damagatchi retest this please

shubham1g5 · 2023-09-30T15:31:46Z

@damagatchi retest this please

shubham1g5 · 2023-09-30T15:34:41Z

@damagatchi retest this please

shubham1g5 · 2023-10-05T19:57:59Z

...care-support-library/src/main/java/org/commcare/commcaresupportlibrary/CommCareLauncher.java

+     * @param context Android context to launch the CommCare with
+     * @param appId Unique Id for CommCare App that CommCare should launch with
+     */
+    public static void launchCommCareForAppIdFromConnect(Context context, String appId) {


Support library should only host methods that are useful for external Apps to integrate with CC, Since this method is connect specific, we should not add it in support library.

shubham1g5 · 2023-10-05T20:05:38Z

app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java

+
+    private void updateUpdatedDate(Date lastUpdate) {
+        DateFormat df = SimpleDateFormat.getDateTimeInstance();
+        updateText.setText(getString(R.string.connect_last_update, df.format(lastUpdate)));


this crashes for me sometimes -

java.lang.IllegalStateException: Fragment ConnectJobsListsFragment{ed114d0} (0884a7a8-bf3a-40a4-a085-088e94a4da8b) not attached to a context. at androidx.fragment.app.Fragment.requireContext(Fragment.java:972) at androidx.fragment.app.Fragment.getResources(Fragment.java:1036) at androidx.fragment.app.Fragment.getString(Fragment.java:1071) at org.commcare.fragments.connect.ConnectJobsListsFragment.updateUpdatedDate(ConnectJobsListsFragment.java:154) at org.commcare.fragments.connect.ConnectJobsListsFragment.-$$Nest$mupdateUpdatedDate(Unknown Source:0) at org.commcare.fragments.connect.ConnectJobsListsFragment$3.processSuccess(ConnectJobsListsFragment.java:132) at org.commcare.activities.connect.ConnectNetworkHelper$1.processSuccess(ConnectNetworkHelper.java:295)```

…android into dv/connect_initial

Not defaulting to null for app record payloads.

…r leaves the page before an API call finishes.

…nto dv/connect_initial

…lavor to override to connect-staging.

…uling hearbeat, do the first request at the time of scheduling

Implements heartbeat for Connect

OrangeAndGreen · 2023-10-31T18:50:31Z

@damagatchi retest this please

…pattern.

…ations.

…ndary phone verification

…user logged in via ConnectID.

…and then new column.

OrangeAndGreen · 2024-08-06T16:42:34Z

@damagatchi retest this please

…silently).

OrangeAndGreen · 2024-08-20T14:54:39Z

@damagatchi retest this please

OrangeAndGreen · 2024-08-21T13:31:14Z

@damagatchi retest this please

…nto dv/connect_initial

OrangeAndGreen · 2024-08-24T02:04:33Z

@damagatchi retest this please

OrangeAndGreen · 2024-08-24T02:31:59Z

@damagatchi retest this please

OrangeAndGreen · 2024-08-25T22:02:25Z

@damagatchi retest this please

…care-android into dv/connect_initial. Fixed some merge conflicts.

OrangeAndGreen · 2024-08-29T14:14:54Z

@damagatchi retest this please

…care-android into dv/connect_initial

…tal exception instead)

OrangeAndGreen · 2024-09-06T15:20:44Z

@damagatchi retest this please

OrangeAndGreen · 2024-09-06T15:44:29Z

@damagatchi retest this please

OrangeAndGreen · 2024-09-06T16:13:27Z

@damagatchi retest this please

OrangeAndGreen · 2024-09-09T14:32:48Z

@damagatchi retest this please

OrangeAndGreen requested review from shubham1g5 and avazirna August 24, 2023 17:34

OrangeAndGreen self-assigned this Aug 24, 2023

shubham1g5 requested changes Aug 29, 2023

View reviewed changes

OrangeAndGreen added this to the 2.55 milestone Sep 14, 2023

shubham1g5 requested changes Sep 24, 2023

View reviewed changes

OrangeAndGreen added the skip-integration-tests Skip android tests. label Oct 3, 2023

shubham1g5 reviewed Oct 5, 2023

View reviewed changes

OrangeAndGreen and others added 15 commits October 30, 2023 08:51

Merge branch 'feature/connect' of https://github.com/dimagi/commcare-…

e72b6be

…android into dv/connect_initial

Made learn progress page scrollable (for small screens).

058e9f6

Not defaulting to null for app record payloads.

Wrapping UI update code in a try-catch so it fails quietly if the use…

376c7c7

…r leaves the page before an API call finishes.

Added null check to job before updating UI.

806ba54

Merge branch 'master' of https://github.com/dimagi/commcare-android i…

752aff9

…nto dv/connect_initial

Going to in-progress jobs list when no available jobs to show.

4b1e3e2

Checking if loginListener is still present before calling back to it.

9010de9

Moved Connect base URL to a build config setting, and added a build f…

722e946

…lavor to override to connect-staging.

Adds connect heartbeat

5f0eff0

change heartbeat to connectID server

a3a183f

Set auth in create network service

0ec3ca0

correct return syntax

d7ff4bf

change the condition to connect enabled instead of unlocked for sched…

64aab67

…uling hearbeat, do the first request at the time of scheduling

Setting custom name for cccStaging build flavor.

ce2fd6c

Merge pull request #2722 from dimagi/connectHeartbeat

d6c1f32

Implements heartbeat for Connect

Disabling Connect heartbeat to work around a failing test.

618f201

OrangeAndGreen added 6 commits July 26, 2024 14:51

Repaired unlock to work when user falls back from fingerprint to PIN/…

c066a24

…pattern.

Added Portuguese translations for Connect strings.

3121cb8

Fixed some errors in Portuguese translations and added missing transl…

9a5e4eb

…ations.

Fixed incorrect navigation when user clicks Change Number during seco…

25e3715

…ndary phone verification

Only showing secondary phone verification tile on app home page when …

30ba43e

…user logged in via ConnectID.

Bug fix: Handling upgrade path properly for adding connect_key table …

e157aef

…and then new column.

OrangeAndGreen added 3 commits August 7, 2024 10:52

Removed catches from ConnectDatabaseUpgrader (so upgrades don't fail …

9407eed

…silently).

Not updating menu item visibility if we can't find it.

dc8692e

Added some safety code to fix a couple crashes.

1ac6665

Merge branch 'master' of https://github.com/dimagi/commcare-android i…

c6a1a9a

…nto dv/connect_initial

OrangeAndGreen changed the base branch from feature/connect to dv/connectid_catchup August 27, 2024 17:25

Merge branch 'dv/connectid_catchup' of https://github.com/dimagi/comm…

d3c6f17

…care-android into dv/connect_initial. Fixed some merge conflicts.

OrangeAndGreen added 4 commits August 29, 2024 15:43

Merge branch 'dv/connectid_catchup' of https://github.com/dimagi/comm…

8eb359b

…care-android into dv/connect_initial

Matching some spacing fixes between branches

3a42bfb

Not swallowing exceptions when parsing server date.

b309170

Ignoring deliveries with exceptions when parsing JSON (logging non-fa…

c2a0499

…tal exception instead)

OrangeAndGreen changed the base branch from dv/connectid_catchup to feature/connect December 5, 2024 19:52

avazirna modified the milestones: 2.55, 2.56 Dec 9, 2024

Connect #2695

Are you sure you want to change the base?

Connect #2695

Conversation

OrangeAndGreen commented Aug 24, 2023 • edited by shubham1g5 Loading

Summary

Product Description

Safety Assurance

Automated test coverage

shubham1g5 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

shubham1g5 commented Sep 15, 2023

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

OrangeAndGreen commented Sep 29, 2023

shubham1g5 commented Sep 30, 2023

shubham1g5 commented Sep 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrangeAndGreen commented Oct 31, 2023

OrangeAndGreen commented Aug 6, 2024

OrangeAndGreen commented Aug 20, 2024

OrangeAndGreen commented Aug 21, 2024

OrangeAndGreen commented Aug 24, 2024

OrangeAndGreen commented Aug 24, 2024

OrangeAndGreen commented Aug 25, 2024

OrangeAndGreen commented Aug 29, 2024

OrangeAndGreen commented Sep 6, 2024

OrangeAndGreen commented Sep 6, 2024

OrangeAndGreen commented Sep 6, 2024

OrangeAndGreen commented Sep 9, 2024

OrangeAndGreen commented Aug 24, 2023 •

edited by shubham1g5

Loading