Require active job in some places #3198
Conversation
… not present. Using requireActiveJob instead of getActiveJob wherever the job is required.
| return activeJob; | ||
| } | ||
|
|
||
| public static ConnectJobRecord requireActiveJob() { |
There was a problem hiding this comment.
Just a question there will be no scenario when user is logged in and will never have active job
There was a problem hiding this comment.
There will. The active job gets set when the user goes into the context of one job, for example to access the learn/deliver app, check progress, etc.
So when the user is on the login page or jobs list (for example), there won't be any active job set. You'll see there are still places where getActiveJob is called and the result is null-checked, i.e. the code expects that a job may or may not be set.
But once we're in a part of the code that requires a job, we should call requireActiveJob instead. For example, the user should never see the delivery progress page when there isn't an active job set, so if that happens we should crash.
|
@OrangeAndGreen This looks really good idea to throw exception whenever no active job is set. This can save lot of our debugging efforts as app will crash with defined error and can be immediately found out in Firebase. I was just thinking that in future, there can be many such checks/getter/setter required for connect job, so having a idea to create some base fragment like |
|
|
||
| public static ConnectJobRecord requireActiveJob() { | ||
| if (activeJob == null) { | ||
| throw new IllegalStateException("No active job set in ConnectManager"); |
There was a problem hiding this comment.
@OrangeAndGreen I am not sure but you are best person to tell if we can add more details here. I am sure we would not like to put any personal info here, but just in case if anything available.
There was a problem hiding this comment.
How about calling Objects.requireNotNull(job) in onCreate of these fragments instead of calling requireJob in every call ? It simplifies overhead of knowing if a fragment expects a job to start with in comparison of monitoring individual calls and deciding where it can be null and where not.
|
The more I think about it, maybe all we want to do here is suppress the code warnings? In that case the code will crash with a null exception whenever a page loads and the active job is null but required (current behavior today). Otherwise, any solution we've discussed, whether relying on requireActiveJob or Object.requireNonNull, just crashes the code a few lines earlier without any additional useful information. |
|
The specific null check (typically done in constructors or init) are generally used to set up developer expectations on the class level around what params are required for proper functioning of the class so that anyone making changes to these classes are aware of those expectations. I agree in respect to the actual app crash, it doesn't really matter much how we do it but it's still good to exit as early as possible. But I do think the provided value is more aligned with code clarity and in-code documentation. |
|
There are 2 main key here
These are my views/opinion: Commcare lacks base views for fragment. It should be there to support such activities like initialize/fetch required objects, null checks, should handle errors like internet connectivity, checking for any permission (instead of implementing in each fragment where it's required), finishing activity with success/failure, logging Firebase view with arguments etc. This is somewhat task, not sure if we need to handle in this ticket or not. For here, if we want to go with simpler solution, check should be made in |
|
Okay, catching and handling early sounds good to me as well. So at this point we've discussed three options:
I'm leaning toward #3, particularly as I could imagine additional uses for the base class in the future. |
…ll-checking active job for fragments that require it. Got rid of requireActiveJob method (using Objects.requireNonNull in any remaining places that need to null-check).
…android into dv/require_active_job Cleaned up some conflicts.
📝 WalkthroughWalkthroughThis change introduces a new abstract superclass, Sequence Diagram(s)sequenceDiagram
participant User
participant Activity
participant ConnectJobFragment
participant ConnectManager
participant ConnectJobRecord
User->>Activity: Navigates to Connect feature
Activity->>ConnectJobFragment: Fragment lifecycle begins (onCreate)
ConnectJobFragment->>ConnectManager: getActiveJob()
ConnectManager-->>ConnectJobFragment: returns ConnectJobRecord
ConnectJobFragment->>ConnectJobFragment: Objects.requireNonNull(job)
ConnectJobFragment-->>Activity: Fragment ready with job instance
Activity->>ConnectJobFragment: Calls methods using job
ConnectJobFragment->>ConnectJobRecord: Accesses job data
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java (1)
47-47: Fix critical null pointer exception in adapter.The
ResultsAdapterdeclares ajobfield but never initializes it. The constructor doesn't accept a job parameter, sojobremains null, causing crashes whenonBindViewHoldercallsjob.getDeliveries()orjob.getPayments().Apply this fix to pass the job to the adapter:
- adapter = new ResultsAdapter(showPayments, getContext()); + adapter = new ResultsAdapter(job, showPayments, getContext());And update the constructor:
- ResultsAdapter(boolean showPayments, Context context) { + ResultsAdapter(ConnectJobRecord job, boolean showPayments, Context context) { + this.job = job; this.showPayments = showPayments; this.context = context; }Also applies to: 57-64
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
71-79: Consider adding null safety check.The inherited
jobfield is used directly without null verification. WhileConnectJobFragmentshould ensure non-null job, consider adding a defensive check for robustness.private void updateSummaryView() { + if (job == null) return; int total = 0; for (ConnectJobPaymentRecord payment : job.getPayments()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/src/org/commcare/activities/StandardHomeActivityUIController.java(3 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectJobDetailBottomSheetDialogFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
🔇 Additional comments (29)
app/src/org/commcare/fragments/connect/ConnectJobFragment.java (1)
13-22: Excellent implementation of the centralized job management pattern.This base class effectively addresses the PR objectives by:
- Centralizing active job retrieval and null checking in one location
- Enforcing consistent failure behavior across all connect fragments
- Reducing code duplication and improving maintainability
- Following the base class approach suggested in the PR discussion
The early null check in
onCreateensures failures occur before UI setup, making debugging easier through clear crash logs.app/src/org/commcare/activities/connect/ConnectActivity.java (2)
37-38: Good addition of required import.The
Objectsimport is correctly added to support the null checking pattern.
74-75: Consistent null safety implementation.The explicit null check follows the established pattern and ensures early failure if no active job is present. The placement immediately after job retrieval and before property access is optimal.
app/src/org/commcare/fragments/connect/ConnectJobDetailBottomSheetDialogFragment.java (2)
22-23: Appropriate import addition.The
Objectsimport is correctly added to support the null checking pattern.
58-59: Consistent null safety pattern applied.The explicit null check maintains consistency with the overall refactoring approach. Since this is a
BottomSheetDialogFragmentthat cannot extend the newConnectJobFragmentbase class, this individual null check is the appropriate solution.app/src/org/commcare/activities/StandardHomeActivityUIController.java (3)
43-43: Required import added correctly.The
Objectsimport supports the null checking pattern used in this file.
82-83: Well-placed null safety check.The null check is appropriately positioned within the conditional block where a job is expected to be present, ensuring early failure if the active job is missing.
113-114: Consistent null checking pattern.The explicit null check follows the same pattern as other locations in this file and across the PR, maintaining consistency in error handling.
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
35-35: Excellent refactoring to use the base class.The change to extend
ConnectJobFragmenteffectively centralizes job management and eliminates redundant code. The fragment now benefits from:
- Centralized job retrieval and null checking in the base class
- Reduced code duplication
- Consistent error handling across all connect fragments
The removal of local job retrieval (previously in
onCreateView) demonstrates the successful consolidation of this logic into the base class.app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (2)
47-48: LGTM! Clean refactoring to use inherited job field.The fragment now uses the inherited
jobfield fromConnectJobFragment, eliminating redundant calls toConnectManager.getActiveJob(). This centralizes job management and reduces code duplication.
32-32: Verify the ConnectJobFragment implementation.The class now extends
ConnectJobFragmentinstead ofFragment, which aligns with the PR objective to centralize job management. However, theConnectJobFragmentclass is not included in the files for review.Please provide the
ConnectJobFragmentimplementation to verify:
- Proper null checking of the active job as mentioned in the PR objectives
- Lifecycle management of the
jobfield- Error handling when no active job is available
#!/bin/bash # Description: Find and display the ConnectJobFragment implementation fd -e java -x grep -l "class ConnectJobFragment" cat $(fd -e java -x grep -l "class ConnectJobFragment")app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (2)
29-29: LGTM! Consistent with the centralized job management refactor.The class now extends
ConnectJobFragment, aligning with the systematic refactoring across all connect-related fragments.
107-113: LGTM! Simplified method using inherited job field.The
getFilteredDeliveries()method now uses the inheritedjobfield directly, removing redundant job retrieval and null checks. This improves code clarity and reduces duplication.app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java (4)
28-28: LGTM! Consistent base class change.The fragment now extends
ConnectJobFragmentfor centralized job management.
72-72: LGTM! Clean usage of inherited job field.The method now uses the inherited
jobfield directly, eliminating redundant job retrieval.
80-80: LGTM! Consistent refactoring.Uses the inherited
jobfield instead of local retrieval.
113-113: LGTM! Consistent usage pattern.The method now uses the inherited
jobfield, maintaining consistency with the refactoring approach.app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (5)
41-41: LGTM! Consistent base class change.The fragment now extends
ConnectJobFragmentfor centralized job management.
64-64: LGTM! Clean usage of inherited job field.The method now uses the inherited
jobfield, eliminating redundant job retrieval.
184-184: LGTM! Consistent refactoring.Uses the inherited
jobfield instead of local retrieval.
198-198: LGTM! Consistent usage pattern.The method now uses the inherited
jobfield.
247-247: LGTM! Consistent refactoring.Uses the inherited
jobfield for payment iteration.app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (2)
28-28: LGTM! Consistent base class change.The fragment now extends
ConnectJobFragmentfor centralized job management.
54-83: LGTM! Comprehensive refactoring to use inherited job field.All methods in this fragment now consistently use the inherited
jobfield fromConnectJobFragment, eliminating redundant job retrievals throughout the class. This improves code maintainability and reduces duplication.app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (2)
39-39: LGTM: Clean refactoring to use base class.The change to extend
ConnectJobFragmentaligns with the PR objective to centralize job management.
58-58: Consider null safety verification.The inherited
jobfield is used without null checks. SinceConnectJobFragmentis not provided in this review, ensure that the base class properly initializes thejobfield inonCreate()beforeonCreateView()is called.Run this script to verify the base class implementation:
#!/bin/bash # Description: Verify ConnectJobFragment implementation and job field initialization # Find the ConnectJobFragment class fd "ConnectJobFragment.java" --exec cat {} # Check for job field initialization patterns ast-grep --pattern 'class ConnectJobFragment { $$$ protected ConnectJobRecord job; $$$ }' # Look for onCreate method with job initialization ast-grep --pattern 'public void onCreate($_) { $$$ job = $$$; $$$ }'app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java (1)
23-23: LGTM: Consistent base class usage.The change to extend
ConnectJobFragmentfollows the established pattern.app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (2)
32-32: LGTM: Proper base class extension.The change to extend
ConnectJobFragmentis consistent with the refactoring pattern.
64-64: Excellent adapter implementation pattern.This shows the correct way to pass the job to the adapter constructor and store it as a field. This pattern should be followed in other fragments like
ConnectResultsListFragment.Also applies to: 87-89
Product Description
No user facing change
Technical Summary
Added requireActiveJob function that returns active job or crashes if not present.
Using requireActiveJob instead of getActiveJob wherever the job is required.
Feature Flag
Connect
Safety Assurance
Safety story
The goal is to crash in a consistent way if functionality is encountered which requires an active job but one isn't set.
Automated test coverage
None
QA Plan
None, this is to help us with debugging when something goes wrong