[New arch] Delete public share #2544

davigonz · 2019-05-10T11:26:16Z

Implements #2517
Needs owncloud/android-library#250

…hanges regarding public share deletion

…ragmentTest

…or local and remote datasources

jcmontesmartos · 2019-05-30T09:46:07Z

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

-                )
-            )
-        )
+        ocShareDao.insert(listOf(newPublicShare()))


Haven't you created a method for a just one item instead of a list?

jcmontesmartos · 2019-05-30T09:46:36Z

.../src/androidTest/java/com/owncloud/android/shares/ui/PublicShareEditionDialogFragmentTest.kt

    private val file = mock(OCFile::class.java)
    private val publicShare = mock(OCShare::class.java)
+    private val expirationDate = 1556575200000


When is it?

I put a comment with the date, but do you think is really important?

I don't know if it will some years later or some years ago.... I don't know...
The problem is if the date is a coming date, it will fail tests just because we are in that date or it's past...
I would do it that date dynamically in order to avoid that issue. For example:
System. currentTimeMillis() + 2000000

In this test we are setting a date in a view and checking if it appears, so I think is not affected by a coming or past date

jcmontesmartos · 2019-05-30T09:48:21Z

owncloudApp/src/main/java/com/owncloud/android/shares/datasource/OCRemoteSharesDataSource.kt

@@ -75,4 +76,11 @@ class OCRemoteSharesDataSource(
        updateRemoteShareOperation.retrieveShareDetails = true
        return updateRemoteShareOperation.execute(client)
    }
+
+    override fun deleteShare(
+        remoteId: Long,


Do you use remoteId anywhere?

Ok, I've seen it below

jcmontesmartos · 2019-05-30T09:50:59Z

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

+    private fun refreshSharesFromStorageManager() {
+        val shareFileFragment = shareFileFragment
+        if (shareFileFragment?.isAdded == true) {   // only if added to the view hierarchy!!
+            shareFileFragment.refreshUsersOrGroupsListFromDB()


it would be better to have a livedata listening data from db. You will save to do this approach ;-)

This is for private shares and has not been touched yet, it will be our next challenge in the architecture.

jcmontesmartos · 2019-05-30T09:54:23Z

owncloudApp/src/main/java/com/owncloud/android/shares/ui/fragment/ShareFileFragment.kt

+        }
+
+        if (!enableMultiplePublicSharing()) {
+            if (publicLinks?.size == 0) {


why don't you use isNullOrEmpty() ?

and the else block wouldn't have condition

jcmontesmartos · 2019-05-30T09:57:11Z

owncloudApp/src/main/java/com/owncloud/android/ui/adapter/SharePublicLinkListAdapter.kt

+ * Adapter to show a list of public links
+ */
+class SharePublicLinkListAdapter(
+    private val mContext: Context, resource: Int, private val mPublicLinks: ArrayList<OCShare>?,


Why don't you put each param in a new line? Something like this:

class SharePublicLinkListAdapter( private val mContext: Context, resource: Int, private val mPublicLinks: ArrayList<OCShare>?, private val mListener: SharePublicLinkAdapterListener )

jcmontesmartos · 2019-05-30T09:57:37Z

owncloudApp/src/main/java/com/owncloud/android/ui/adapter/SharePublicLinkListAdapter.kt

+) : ArrayAdapter<OCShare>(mContext, resource) {
+
+    override fun getCount(): Int {
+        return mPublicLinks!!.size


it can be null!!!

override fun getCount(): Int = mPublicLinks?.size ?: 0

better?

jcmontesmartos · 2019-05-30T09:59:40Z

owncloudApp/src/main/java/com/owncloud/android/ui/adapter/SharePublicLinkListAdapter.kt

+            val share = mPublicLinks[position]
+
+            // If there's no name, set the token as name
+            view.publicLinkName.text = if (share.name?.isEmpty()!!) share.token else share.name


if name can be null, you can't set isEmpty as not null. That doesn't make any sense.

I would use isNullOrEmpty function instead

You are right, it does not make any sense and Android Studio keeps doing it when converting java files to kotlin 😄 , I will use if (share.name?.isEmpty() == true) instead

and why don't you use if (share.name.isNullOrEmpty()) better?

jcmontesmartos

LGTM now

davigonz · 2019-05-30T12:27:53Z

Code approved, ready to test @jesmrec

jesmrec · 2019-05-31T12:23:33Z

Approved

This was referenced May 10, 2019

[New arch] Delete public share owncloud/android-library#250

Merged

[New architecture] Delete public shares #2517

Closed

davigonz force-pushed the new_arch/delete_public_share branch 2 times, most recently from 2b471b0 to b7e58b8 Compare May 16, 2019 10:56

davigonz force-pushed the new_arch/shares branch 2 times, most recently from 9421e7f to 6f24ea1 Compare May 24, 2019 11:31

davigonz added 20 commits May 24, 2019 13:44

Include delete share operation in dao and datasources

db8d4f7

Convert RemoveShareDialogFragment into Kotlin and include some more c…

4a914c3

…hanges regarding public share deletion

Use listeners to properly show remove share fragment

4b6c4dc

Delete share from ShareActivity

2b1badb

Clean up some dependencies when getting public shares and capabilities

818ad27

Clean up dependencies when creating/editing shares

9911cec

Dismiss dialog to edit shares

0faf095

Get rid of account dependency in PublicShareDialogFragment

8ea1bd4

Fix unit tests

3cc62ae

Fixing UI tests after some dependencies cleanup[WIP]

291f174

Fix ShareFileFragmentTest UI tests

ad23338

Fix crash when getting shares the first time

aa6f5ae

Fix PublicShareCreationDialogFragmentTest UI tests

302d8d1

Fix UI tests in PublicShareEditionDialogFragmentTest and ShareFolderF…

60eaaed

…ragmentTest

Use room implementation in Content Provider to delete shares + test f…

993ddf6

…or local and remote datasources

Refactor share repository tests

64d34ed

Include tests for public sharing deletion in repository

0e37f98

Move filePath and account variables to ShareRepository creation

ef99a02

Tests for sharing deletion within OCShareViewmodel and OCShareDao

fe8723f

Swap expected and actual parameters for every assertEquals call

6e01457

davigonz force-pushed the new_arch/delete_public_share branch from 3324c21 to 6e01457 Compare May 24, 2019 11:45

davigonz changed the title ~~[New arch] Delete public share [WIP]~~ [New arch] Delete public share May 24, 2019

Delete unused list

4482591

jcmontesmartos reviewed May 30, 2019

View reviewed changes

davigonz added 2 commits May 30, 2019 13:14

Apply CR changes

3d3669b

Apply additional changes requested in CR

27fe03c

jcmontesmartos approved these changes May 30, 2019

View reviewed changes

davigonz self-assigned this May 30, 2019

davigonz added the code approved label May 30, 2019

jesmrec added the Approved by qa label May 31, 2019

jesmrec self-assigned this May 31, 2019

jesmrec self-requested a review May 31, 2019 12:23

jesmrec approved these changes May 31, 2019

View reviewed changes

davigonz merged commit 3f2b230 into new_arch/shares Jun 3, 2019

delete-merged-branch bot deleted the new_arch/delete_public_share branch June 3, 2019 06:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New arch] Delete public share #2544

[New arch] Delete public share #2544

davigonz commented May 10, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019 •

edited

Loading

jcmontesmartos May 30, 2019

jcmontesmartos May 30, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019

jcmontesmartos May 30, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019

davigonz May 30, 2019

jcmontesmartos May 30, 2019

jcmontesmartos left a comment

davigonz commented May 30, 2019

jesmrec commented May 31, 2019

[New arch] Delete public share #2544

[New arch] Delete public share #2544

Conversation

davigonz commented May 10, 2019

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

jcmontesmartos May 30, 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

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

jcmontesmartos left a comment

Choose a reason for hiding this comment

davigonz commented May 30, 2019

jesmrec commented May 31, 2019

jcmontesmartos May 30, 2019 •

edited

Loading