[New arch] Private shares: edition and deletion #2603

davigonz · 2019-07-08T13:19:22Z

Implements #2556

Bugs and improvements

(1) Loading dialog stuck when trying to create an already existing private share [FIXED]

…e shares

davivel · 2019-07-12T09:03:19Z

owncloudApp/src/androidTest/java/com/owncloud/android/shares/data/OCLocalShareDataSourceTest.kt

@@ -236,8 +261,12 @@ class OCLocalDataSourceTest {
        assertEquals(8, updatedShareId)
    }

+    /**************************************************************************************************************
+     *************************************************** COMMON ***************************************************


Common to what?

Common to private and public shares

davivel · 2019-07-12T09:07:51Z

owncloudApp/src/androidTest/java/com/owncloud/android/shares/data/OCShareDaoTest.kt

+
+        val textShares = getValue(
+            ocShareDao.getSharesAsLiveData(
+                "/Texts/text1.txt", "admin@server", listOf(ShareType.USER.value)


This is a bit difficult to read. How should I know that "/Texts/text1.txt", "admin@server", etc... correspond to the fields of the share returned by 'createDefaultPrivasteShare()' ?

I would save the value of createDefaultPrivateShare() in a variable at the beginning, then use the fields of that variable here to pass file, user, etc... That way it's clear that we are getting the same share that we inserted first.

davivel · 2019-07-12T09:08:39Z

owncloudApp/src/androidTest/java/com/owncloud/android/shares/data/OCShareDaoTest.kt

+    fun deletePrivateShare() {
+        ocShareDao.insert(createDefaultPrivateShare())
+
+        ocShareDao.deleteShare(1)


Similar here, how do we know that the Share created by 'createDefaultPrivarteShare' is the one with id '1'?

davivel · 2019-07-12T09:13:13Z

.../androidTest/java/com/owncloud/android/shares/domain/privateShares/DeletePrivateShareTest.kt

+        `when`(ocShareViewModel.getPrivateShares(file.remotePath)).thenReturn(privateSharesLiveData)
+        `when`(ocShareViewModel.getPublicShares(file.remotePath)).thenReturn(MutableLiveData())
+
+        stopKoin()


I would create a method with annotation @after to put 'stopKoin()' there instead.

I could create it to stop Koin for the last executed test but this line would be still needed because since instrumented tests are launched on a device, the android application class is instantiated and it causes Koin initialization as a side effect. Therefore, we need to stop it here before starting koin again, otherwise I get the exception below:

org.koin.core.error.KoinAppAlreadyStartedException: A Koin Application has already been started

You can create a full Koin tree to be used only in tests and load it in the @Begin methods with StandAloneContext.loadKoinModules so that if overrides the full app module(s), then keep the @Begin/@After symmetry.

davivel · 2019-07-12T09:19:07Z

.../androidTest/java/com/owncloud/android/shares/domain/privateShares/DeletePrivateShareTest.kt

+            )
+        ).perform(click())
+
+        privateSharesLiveData.postValue(


Sorry, I don't understand what is this for.

We are mocking the viewmodel so the operation to delete the share from localShareDataSource, which takes place in the repository, is never executed. Therefore, privateSharesLiveData is not updated after the deletion so we have to do it here manually.

I know this makes these tests to be a little "fake", so I guess we would need to use mocks in layers below, i.e. datasources, to have the whole flow. Shares will not be released without #2523 so I've just created a new task there to face this change.

davivel · 2019-07-12T09:25:09Z

owncloudApp/src/main/java/com/owncloud/android/shares/domain/OCShareRepository.kt

+                }
+
+                if (sharesFromServer.isEmpty()) {
+                    localShareDataSource.deleteSharesForFile(filePath, accountName)


Is this needed? Shouldn't be done inside 'replaceShares(...)' right below?

The replaceShares method in OCShareDao.kt includes the code below and its aim is replacing the still existing shares, not deleting the shares that no longer exist in the server.

@Transaction open fun replaceShares(ocShares: List<OCShare>): List<Long> { for (ocShare in ocShares) { deleteSharesForFile(ocShare.path, ocShare.accountOwner) } return insert(ocShares) }

davivel · 2019-07-12T09:29:26Z

owncloudApp/src/main/java/com/owncloud/android/shares/presentation/ShareActivity.kt

@@ -114,7 +112,7 @@ class ShareActivity : FileActivity(), ShareFragmentListener {
        setContentView(R.layout.share_activity)

        // Set back button
-        supportActionBar!!.setDisplayHomeAsUpEnabled(true)
+        supportActionBar?.setDisplayHomeAsUpEnabled(true)


What happens if supportActionBar is null?

The arrow to go back would not appear in the action bar but you could still use the back button in the Android navigation bar.

…ivate share

davigonz changed the base branch from master to new_arch/shares July 8, 2019 13:19

davigonz mentioned this pull request Jul 8, 2019

[New arch] Edit and delete private shares #2556

Closed

3 tasks

davigonz force-pushed the new_arch/private_shares_edit_remove branch from 43249ba to a5e728f Compare July 9, 2019 14:34

davigonz added 8 commits July 10, 2019 10:09

Initial commit

e23b5f9

Show permissions and edit [WIP]

b3eba6a

Fix UI glitch when refreshing share list

2586582

Implement private shares deletion

5f13807

Unit tests and part of instrumented tests to update and delete privat…

09b4d3a

…e shares

UI tests for EditPrivateShareFragment

df7ab61

UI tests for private shares loading and errors

5ecf2a0

UI tests for deleting private shares

21302cc

davigonz force-pushed the new_arch/private_shares_edit_remove branch from f59a586 to 21302cc Compare July 10, 2019 08:12

davigonz changed the title ~~[New arch] Private shares: edition and deletion [WIP]~~ [New arch] Private shares: edition and deletion Jul 10, 2019

davivel suggested changes Jul 12, 2019

View reviewed changes

Apply CR changes

dfe4dd2

davivel approved these changes Jul 15, 2019

View reviewed changes

davigonz mentioned this pull request Jul 15, 2019

[New arch] App modularization and decoupling #2523

Closed

davigonz self-assigned this Jul 15, 2019

Fix loading dialog stuck when trying to create an already existing pr…

5e802bb

…ivate share

davigonz added code approved Approved by qa labels Jul 15, 2019

davigonz merged commit fd5046e into new_arch/shares Jul 15, 2019

delete-merged-branch bot deleted the new_arch/private_shares_edit_remove branch July 15, 2019 12:46

davigonz mentioned this pull request Jul 29, 2019

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

Closed

25 tasks

jesmrec mentioned this pull request Nov 5, 2019

[New arch] Modularization tests (adapt existing, and create new ones) + FileDataStorageManager refactor #2701

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New arch] Private shares: edition and deletion #2603

[New arch] Private shares: edition and deletion #2603

davigonz commented Jul 8, 2019 •

edited

Loading

davivel Jul 12, 2019

davigonz Jul 12, 2019

davivel Jul 12, 2019

davigonz Jul 12, 2019

davivel Jul 12, 2019

davigonz Jul 12, 2019

davivel Jul 12, 2019

davigonz Jul 12, 2019 •

edited

Loading

davivel Jul 15, 2019 •

edited

Loading

davivel Jul 12, 2019

davigonz Jul 12, 2019 •

edited

Loading

davivel Jul 12, 2019

davigonz Jul 12, 2019

davivel Jul 12, 2019

davigonz Jul 12, 2019

[New arch] Private shares: edition and deletion #2603

[New arch] Private shares: edition and deletion #2603

Conversation

davigonz commented Jul 8, 2019 • edited Loading

Bugs and improvements

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

davigonz Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

davivel Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davigonz Jul 12, 2019 • edited Loading

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

davigonz commented Jul 8, 2019 •

edited

Loading

davigonz Jul 12, 2019 •

edited

Loading

davivel Jul 15, 2019 •

edited

Loading

davigonz Jul 12, 2019 •

edited

Loading