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.

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

[New arch] Modularization, decoupling and new error handling #2607

Closed
24 of 25 tasks
davigonz opened this issue Jul 29, 2019 · 6 comments
Closed
24 of 25 tasks

[New arch] Modularization, decoupling and new error handling #2607

davigonz opened this issue Jul 29, 2019 · 6 comments

Comments

@davigonz
Copy link
Contributor

davigonz commented Jul 29, 2019

Specific issue to start modularizing the app, following the guidelines defined in #2351

  • Tasks
    • Split up code into different modules following a layers approach and move the corresponding shares code to those modules:
      • Presentation layer => owncloudApp module
      • Domain layer => owncloudDomain module
      • Data layer => owncloudData module
    • Use mockk instead of mockito in UI and domain unit tests, more prepared to work with Kotlin instead of Java.
    • Transform sharing operations into usecases in domain model.
    • Decouple data and domain modules with the help of dependency injection (Koin). Domain should not depend on data layer directly => Difficult to achieve due to strong ownCloud Client dependencies that force us to use instances of repositories in domain layer when we should use repository interfaces.
    • Create share data and domain models with some Mapper classes to transform them.
    • Create capability data and domain models with some Mapper classes to transform them.
    • Error handling, + info in [SPIKE] Error and data handling inside new architecture modularization #2665
    • Create new services as facade for remote operations to decouple that logic from remote datasources and ease remote datasource and repository tests
    • Fix already existing tests: unit and UI. Use corresponding share models in TestUtils.
      • Data layer
        • Datasources
        • Repositories
        • Mappers (model-entities, model-remote)
        • Entity
        • DAOs
      • Domain layer
        • Usecases
      • Presentation layer
        • ViewModel Tests
        • UI tests
    • New tests for usecases
    • New tests for models
    • Use StandAloneContext.loadKoinModules. See [New arch] Private shares: edition and deletion #2603 (comment)

PRs

Pending issues (create separate ones?)

  • Consider adding a new module just for data testing.
  • Consider renaming usecases to Fetch...() instead of Refresh...FromServer()

Bugs and improvements

@davigonz davigonz added this to the 2.12.0 milestone Jul 29, 2019
@davigonz davigonz self-assigned this Jul 29, 2019
@jesmrec jesmrec removed the Sprint label Jul 30, 2019
@jesmrec jesmrec modified the milestones: 2.12.0, 2.13.0 Jul 30, 2019
@jesmrec jesmrec modified the milestones: 2.13.0, 2.14.0 Aug 27, 2019
@davigonz davigonz changed the title [New arch] Shares modularization and decoupling [New arch] Modularization and decoupling Oct 2, 2019
@davigonz davigonz changed the title [New arch] Modularization and decoupling [New arch] Modularization, decoupling and new error handling Oct 2, 2019
@davigonz
Copy link
Contributor Author

davigonz commented Nov 6, 2019

[BUG] (1) Error is shown again when rotating the device

  1. Open the application with Activity or Fragment
  2. Cause an error, e.g. switch airplane mode on and tap a button that makes a request to server
  3. Observe the error is shown
  4. Rotate the device

Current behavior: Error is shown again and the user has not done any request.
Expected behavior: No error is shown

@davigonz
Copy link
Contributor Author

davigonz commented Nov 6, 2019

[FIXED] (2) Users and groups search doesn't work

  1. Tap share icon in a file.
  2. Tap + button in users and groups section.
  3. Type several characters to search users or groups.

Current behavior: No results
Expected behavior: Users or groups appear

2019-11-06 13:19:05.425 26092-26488/com.owncloud.android.debug W/SuggestionsAdapter: Search suggestions query threw an exception. java.lang.IllegalStateException: Cannot invoke observeForever on a background thread at androidx.lifecycle.LiveData.assertMainThread(LiveData.java:461) at androidx.lifecycle.LiveData.observeForever(LiveData.java:222) at com.owncloud.android.presentation.viewmodels.capabilities.OCCapabilityViewModel.<init>(OCCapabilityViewModel.kt:69) at com.owncloud.android.presentation.viewmodels.capabilities.OCCapabilityViewModel.<init>(OCCapabilityViewModel.kt:49) at com.owncloud.android.presentation.providers.sharing.UsersAndGroupsSearchProvider.searchForUsersOrGroups(UsersAndGroupsSearchProvider.kt:128) at

@davigonz
Copy link
Contributor Author

Merged, great job guys!! @abelgardep @jesmrec @jcmontesmartos

giphy

@hannesa2
Copy link
Contributor

Are there plans to convert/replace UserProfilesRepository , FileDataStorageManager and ProviderMeta by a room solution ?

@abelgardep
Copy link
Contributor

Are there plans to convert/replace UserProfilesRepository , FileDataStorageManager and ProviderMeta by a room solution ?

For sure, the idea is to migrate the app completely to the new architecture. That means using Room as it is already done with shares functionality. Step by step 👍

@davigonz
Copy link
Contributor Author

davigonz commented Dec 12, 2019

In addition to what @abelgardep says

Are there plans to convert/replace UserProfilesRepository , FileDataStorageManager and ProviderMeta by a room solution ?

Yes, but is going to be done by features, I mean:

- FileDataStorageManager: we already removed all the sharing methods from there (~500 lines). We will continue replacing the old implementation in coming issues: files, uploads, camera uploads...

- ProviderMeta: the idea is to keep it in the data layer but just with information required by the room solution.

- UserProfilesRepository: same, we will convert it.

By the way, now that the new architecture is on master, any contribution is welcome 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants