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

Jump to bottom

Simplify handling for showing terms of service dialog #3033

Closed
wants to merge 14 commits into from
Closed

Simplify handling for showing terms of service dialog #3033

wants to merge 14 commits into from

Conversation

shobhitagarwal1612
Copy link
Member

Fixes #3031

Things done:

  • Removes redundant error validation checks as the collect button is already hidden if user doesn't have permissions or loi has not tasks
  • Replaces corountine flow with a getter call
  • Code readability improvements

See description of #3031 for more details.

Verified for regressions by running the app locally covering all above mentioned edge cases.

@gino-m @anandwana001 PTAL?

@shobhitagarwal1612
Pull data classes out of sealed interface
a89f5ce
@shobhitagarwal1612
Reuse hasNonLoiTasks() for validating if job has non-loi tasks
10e5ac1
@shobhitagarwal1612
Refactor: Remove redundant flow for data sharing terms
fc47572 
* Remove `activeSurveyDataSharingTermsFlow` flow and instead use a direct method call.
* Remove unnecessary `cancellable()` call.
* This is in preparation for removing the data sharing terms dialog.
@shobhitagarwal1612
Refactor data collection button click handling
c1bda4d 
* Remove unused parameter `hasDataSharingTerms` from `onCollectData()`
* Fetch data sharing terms directly in `onCollectData()`
* Handle null data sharing terms
* Show data sharing terms dialog only if terms are present and valid
@shobhitagarwal1612
Directly use active survey from repository instead of subscribing to …
7e8082c 
…flow when accepting data sharing consent
@shobhitagarwal1612
Call hasValidTasks internally within onCollectData
5efa361
@shobhitagarwal1612
Remove the permission check for data collection in onCollectData()
667a26a 
We hide the collect button if the user doesn't have permissions. So this logic will never be called.
@shobhitagarwal1612
Remove 'no tasks' error string and its usage as the collect button is…
400eb60 
… hidden if loi has no tasks

#2598
@shobhitagarwal1612
Move checking for invalid data sharing terms to view model. Also, wra…
b3d9890 
…p the response in Result for error handling.
@shobhitagarwal1612
Pass user access into MapCardUiData
4e869e0
@shobhitagarwal1612
Modified MapCardAdapter to remove updateSubmissionCount from its …
5622175 
…constructor
@auto-assign auto-assign bot requested a review from sufyanAbbasi February 4, 2025 18:51
@codecov Codecov
Copy link

codecov bot commented Feb 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 15.78947% with 64 lines in your changes missing coverage. Please review.

Project coverage is 64.15%. Comparing base (bc73c41) to head (eeb851e).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
...ome/mapcontainer/HomeScreenMapContainerFragment.kt 3.33% 26 Missing and 3 partials ⚠️
...me/mapcontainer/HomeScreenMapContainerViewModel.kt 20.68% 23 Missing ⚠️
...ground/ui/home/mapcontainer/cards/MapCardUiData.kt 0.00% 11 Missing ⚠️
...google/android/ground/repository/UserRepository.kt 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master    #3033   +/-   ##
=========================================
  Coverage     64.15%   64.15%           
+ Complexity     1266     1265    -1     
=========================================
  Files           270      270           
  Lines          6481     6481           
  Branches        901      902    +1     
=========================================
  Hits           4158     4158           
  Misses         1730     1730           
  Partials        593      593
Files with missing lines Coverage Δ
.../main/java/com/google/android/ground/model/Role.kt 100.00% <100.00%> (+20.00%) ⬆️
...google/android/ground/repository/UserRepository.kt 86.11% <80.00%> (+0.81%) ⬆️
...ground/ui/home/mapcontainer/cards/MapCardUiData.kt 0.00% <0.00%> (ø)
...me/mapcontainer/HomeScreenMapContainerViewModel.kt 41.46% <20.68%> (-4.37%) ⬇️
...ome/mapcontainer/HomeScreenMapContainerFragment.kt 17.93% <3.33%> (+0.35%) ⬆️

@shobhitagarwal1612
Copy link
Member Author

Will add some test coverage since the diff is only 8% covered.

@shobhitagarwal1612
Copy link
Member Author

Marking as draft since it conflicts with #2892

@shobhitagarwal1612 shobhitagarwal1612 marked this pull request as draft February 5, 2025 11:53
@shobhitagarwal1612
Copy link
Member Author

Done in #3039

@shobhitagarwal1612 shobhitagarwal1612 deleted the ashobhit/3031/homescreenfragment branch February 12, 2025 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anandwana001 anandwana001 anandwana001 approved these changes

@sufyanAbbasi sufyanAbbasi Awaiting requested review from sufyanAbbasi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Code health] Cleanup logic for showing terms of service dialog
2 participants
@shobhitagarwal1612 @anandwana001