[New arch] Use moshi to parse sharees #352

theScrabi · 2020-10-14T07:59:37Z

related issue: owncloud/android#2704
related app pr: owncloud/android#2994

abelgardep

Some changes requested @theScrabi

owncloudComLibrary/build.gradle

owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/CommonOcsResponse.kt

abelgardep · 2020-10-16T13:12:21Z

owncloudComLibrary/src/test/java/com/owncloud/android/lib/ShareeResponseTest.kt

+        val EXAMPLE_RESPONSE = """
+{
+    "ocs": {
+        "data": {
+            "exact": {
+                "groups": [],


I like it, but i think that we could create a new responses package with json files and import those files in the test. That way we will have all responses in the same package as real json and not in companion as strings. What do you think? I could help to do it

Sounds good

Ok, tell me if you need some help to do it

owncloudComLibrary/src/test/java/com/owncloud/android/lib/ShareeResponseTest.kt

abelgardep · 2020-10-16T13:20:15Z

...Library/src/main/java/com/owncloud/android/lib/resources/shares/GetRemoteShareesOperation.kt

-        private const val NODE_EXACT = "exact"
-        private const val NODE_USERS = "users"
-        private const val NODE_GROUPS = "groups"
-        private const val NODE_REMOTES = "remotes"
        const val NODE_VALUE = "value"
        const val PROPERTY_LABEL = "label"
        const val PROPERTY_SHARE_TYPE = "shareType"


Some constants are not used anywhere, could you take a look?

Those properties are not used anymore, you parse it directly using moshi. I think you can remove them.

...Library/src/main/java/com/owncloud/android/lib/resources/shares/GetRemoteShareesOperation.kt

...mLibrary/src/main/java/com/owncloud/android/lib/resources/shares/responses/ShareeResponse.kt

owncloudComLibrary/src/test/java/com/owncloud/android/lib/ShareeResponseTest.kt

abelgardep

@theScrabi Tiny suggestions

abelgardep · 2020-10-20T07:07:00Z

...Library/src/main/java/com/owncloud/android/lib/resources/shares/GetRemoteShareesOperation.kt

-        private const val NODE_EXACT = "exact"
-        private const val NODE_USERS = "users"
-        private const val NODE_GROUPS = "groups"
-        private const val NODE_REMOTES = "remotes"
        const val NODE_VALUE = "value"
        const val PROPERTY_LABEL = "label"
        const val PROPERTY_SHARE_TYPE = "shareType"


Those properties are not used anymore, you parse it directly using moshi. I think you can remove them.

abelgardep · 2020-10-20T07:12:37Z

owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/shares/RemoteShare.kt

+        private val values = values();
+        fun fromValue(value: Int) = values.firstOrNull { it.value == value }


Suggested change

private val values = values();

fun fromValue(value: Int) = values.firstOrNull { it.value == value }

fun fromValue(value: Int) = values().firstOrNull { it.value == value }

abelgardep

Take a look at the failing tests and then we move it forward 🚀 @theScrabi

abelgardep · 2020-10-20T13:18:36Z

...Library/src/main/java/com/owncloud/android/lib/resources/shares/GetRemoteShareesOperation.kt

-        private const val NODE_GROUPS = "groups"
-        private const val NODE_REMOTES = "remotes"
        const val NODE_VALUE = "value"
        const val PROPERTY_LABEL = "label"
        const val PROPERTY_SHARE_TYPE = "shareType"


There are some constants that are not used anymore. Please, remove them. @theScrabi

abelgardep · 2020-10-20T13:20:11Z

.../resources/com.owncloud.android.lib.resources.sharees.responses/example_sharee_response.json

@@ -0,0 +1,60 @@
+{


I like it 🎉 🎉 But tests failed, could you take a look? 👍

abelgardep

I have some questions here @theScrabi

abelgardep · 2020-10-21T15:27:02Z

...rary/src/test/java/com/owncloud/android/lib/resources/shares/responses/ShareeResponseTest.kt

+
+    @Test
+    fun `check structure - ok - contains users`() {
+        assertNotEquals(null, response.ocs.data.users)


If we are using the same response on each test, why shouldn't we unify them in a single one?

assertNull
assertNotNull 🤔

Please correct me if I am wrong, but I learned that every test should test one thing and not everything. Am I wrong?

abelgardep · 2020-10-21T15:30:37Z

...rary/src/test/java/com/owncloud/android/lib/resources/shares/responses/ShareeResponseTest.kt

+    fun `check empty response - ok - parsing ok`() {
+        val moshi = Moshi.Builder().build()
+        val type: Type = Types.newParameterizedType(CommonOcsResponse::class.java, ShareeOcsResponse::class.java)
+        val adapter: JsonAdapter<CommonOcsResponse<ShareeOcsResponse>> = moshi.adapter(type)
+        loadResponses(EMPTY_RESPONSE_JSON, adapter)!!
+    }


To test this, you have a Before where you load a EXAMPLE_RESPONSE_JSON and then you load an empty one. What are you testing here? I cannot see any assert 🤔

abelgardep · 2020-10-21T15:35:39Z

...rary/src/test/java/com/owncloud/android/lib/resources/shares/responses/ShareeResponseTest.kt

+    @Before
+    fun prepare() {
+        val moshi = Moshi.Builder().build()
+        val type: Type = Types.newParameterizedType(CommonOcsResponse::class.java, ShareeOcsResponse::class.java)
+        val adapter: JsonAdapter<CommonOcsResponse<ShareeOcsResponse>> = moshi.adapter(type)
+        response = loadResponses(EXAMPLE_RESPONSE_JSON, adapter)!!
+    }


I would load responses on each test depending on the testing purpose. You can move adapter to a lateinit var for example, and on each test, simply call: loadResponses(EXAMPLE_RESPONSE_JSON) and assert that everything is fine with that response.

Suggested change

@Before

fun prepare() {

val moshi = Moshi.Builder().build()

val type: Type = Types.newParameterizedType(CommonOcsResponse::class.java, ShareeOcsResponse::class.java)

val adapter: JsonAdapter<CommonOcsResponse<ShareeOcsResponse>> = moshi.adapter(type)

response = loadResponses(EXAMPLE_RESPONSE_JSON, adapter)!!

}

@Before

fun prepare() {

val moshi = Moshi.Builder().build()

val type: Type = Types.newParameterizedType(CommonOcsResponse::class.java, ShareeOcsResponse::class.java)

val adapter: JsonAdapter<CommonOcsResponse<ShareeOcsResponse>> = moshi.adapter(type)

}

abelgardep · 2020-10-21T15:36:54Z

...rary/src/test/java/com/owncloud/android/lib/resources/shares/responses/ShareeResponseTest.kt

+    @Test
+    fun `check structure - ok - example response files exist`() {
+        val file = File(EXAMPLE_RESPONSE_JSON)
+        assertTrue(file.exists())
+    }


From my point of view, this test is not needed. If json do not exists, every test will fail, not only this one.

You are right, but this way you can directly see what happens.

abelgardep

Approved on my side

delete unused const values bla

theScrabi mentioned this pull request Oct 14, 2020

[New arch] Use Moshi to parse sharees owncloud/android#2704

Closed

6 tasks

theScrabi requested a review from abelgardep October 14, 2020 09:09

theScrabi force-pushed the new_arch/moshi_parse_sharees branch from 8e52305 to 88cb76b Compare October 14, 2020 11:21

theScrabi mentioned this pull request Oct 15, 2020

[New arch] Use moshi to parse sharees owncloud/android#2994

Merged

theScrabi marked this pull request as ready for review October 16, 2020 09:53

abelgardep mentioned this pull request Oct 16, 2020

New arch/moshi parse shares #350

Closed

abelgardep suggested changes Oct 16, 2020

View reviewed changes

abelgardep reviewed Oct 19, 2020

View reviewed changes

...mLibrary/src/main/java/com/owncloud/android/lib/resources/shares/responses/ShareeResponse.kt Outdated Show resolved Hide resolved

abelgardep reviewed Oct 19, 2020

View reviewed changes

owncloudComLibrary/src/test/java/com/owncloud/android/lib/ShareeResponseTest.kt Outdated Show resolved Hide resolved

theScrabi requested a review from abelgardep October 19, 2020 13:11

abelgardep suggested changes Oct 20, 2020

View reviewed changes

theScrabi force-pushed the new_arch/moshi_parse_sharees branch from 797ca0d to a9e0ee6 Compare October 20, 2020 12:35

theScrabi requested a review from abelgardep October 20, 2020 12:42

abelgardep suggested changes Oct 20, 2020

View reviewed changes

theScrabi force-pushed the new_arch/moshi_parse_sharees branch 2 times, most recently from 77739ef to 9c9a225 Compare October 21, 2020 14:18

theScrabi requested a review from abelgardep October 21, 2020 14:46

abelgardep suggested changes Oct 21, 2020

View reviewed changes

theScrabi requested a review from abelgardep October 22, 2020 11:14

abelgardep approved these changes Oct 22, 2020

View reviewed changes

abelgardep assigned theScrabi Oct 22, 2020

abelgardep added the code approved label Oct 22, 2020

theScrabi added 8 commits October 29, 2020 12:37

move responses models into module folder

8a04231

clean run method by braking down into small functions

4f72f93

add Sharee response model

9f1c20b

add support for tests

6bdb2ba

test model

1c08a94

update GetRemoteShareesOperation to use moshi

b866384

update shareeservice to use ShareeOcsResponse

fe425c8

get flat representation from ShareeOcsResponse

7402c89

theScrabi added 10 commits October 29, 2020 12:37

return sharees without exact match

26def4f

add aditional data field to ocs sharees

90b6ff5

make sharee parts non null that can't be null

34bf83d

disable unitTestBinaryResource as its deprecated

29de5ab

refactor ShareType

db478ef

add changes according to review

76c55c9

move response json to resource folder

6ec1bd2

apply suggested changes

257494a

delete unused const values bla

apply changes acording to review

ce9fc3f

apply changes acording to review

68b8877

abelgardep force-pushed the new_arch/moshi_parse_sharees branch from fdb373f to 68b8877 Compare October 29, 2020 11:37

abelgardep changed the title ~~New arch/moshi parse sharees~~ [New arch] Use moshi to parse sharees Oct 29, 2020

abelgardep merged commit af579a3 into master Oct 29, 2020

delete-merged-branch bot deleted the new_arch/moshi_parse_sharees branch October 29, 2020 12:15

abelgardep mentioned this pull request Nov 4, 2020

Use Moshi to parse shares #356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New arch] Use moshi to parse sharees #352

[New arch] Use moshi to parse sharees #352

theScrabi commented Oct 14, 2020 •

edited

Loading

abelgardep left a comment

abelgardep Oct 16, 2020

theScrabi Oct 19, 2020

abelgardep Oct 20, 2020

abelgardep Oct 16, 2020

abelgardep Oct 20, 2020

abelgardep left a comment

abelgardep Oct 20, 2020

abelgardep Oct 20, 2020

abelgardep left a comment

abelgardep Oct 20, 2020

abelgardep Oct 20, 2020

abelgardep left a comment

abelgardep Oct 21, 2020

theScrabi Oct 22, 2020

abelgardep Oct 21, 2020

abelgardep Oct 21, 2020

abelgardep Oct 21, 2020

theScrabi Oct 22, 2020

abelgardep left a comment

		private val values = values();
		fun fromValue(value: Int) = values.firstOrNull { it.value == value }

	private val values = values();
	fun fromValue(value: Int) = values.firstOrNull { it.value == value }
	fun fromValue(value: Int) = values().firstOrNull { it.value == value }

[New arch] Use moshi to parse sharees #352

[New arch] Use moshi to parse sharees #352

Conversation

theScrabi commented Oct 14, 2020 • edited Loading

abelgardep left a 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

abelgardep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelgardep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelgardep left a 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

abelgardep left a comment

Choose a reason for hiding this comment

theScrabi commented Oct 14, 2020 •

edited

Loading