Add is editable api field #2435

atorresveiga · 2022-06-01T16:29:18Z

Description

This PR adds the is_editable field to the database and uses a fallback strategy for older stores.

Testing instructions

run testMigrate15to16
run OrderDtoMapperTest
For a full set of tests check the Woo PR

atorresveiga · 2022-06-01T18:53:36Z

...ce/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/order/OrderDtoMapper.kt

                    metaData = this.meta_data.toString(),
-                    paymentUrl = this.payment_url ?: ""
+                    paymentUrl = this.payment_url ?: "",
+                    isEditable = this.is_editable ?: this.status in EDITABLE_STATUSES


For older stores < v6.6 we check if the status is "pending", "on-hold", "auto-draft"

peril-wordpress-mobile · 2022-06-01T22:05:28Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

wzieba

I left some comments, WDYT?

wzieba · 2022-06-02T08:57:01Z

...ins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt

        ).allowMainThreadQueries()
                .fallbackToDestructiveMigrationOnDowngrade()
-                .fallbackToDestructiveMigrationFrom(1, 2)
+                .fallbackToDestructiveMigrationFrom(1, 2, 15)


If I understand correctly, you added version 15 here to force the app to re-fetch Orders and assert that isEditable is assigned, right?

Maybe instead of recreating the whole database, we could add a migration that will remove only Orders? It should be fairly straightforward, won't remove entities that are not affected, and will document the decision better (we'll know in the future what changes exactly migration from 15 to 16 caused). WDYT?

wzieba · 2022-06-02T08:57:56Z

plugins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/model/OrderEntity.kt

+    @ColumnInfo(name = "isEditable", defaultValue = "true")
+    val isEditable: Boolean = false


Could you share more context why defaultValue and default constructor values differ?

wzieba · 2022-06-02T09:01:09Z

plugins/woocommerce/schemas/org.wordpress.android.fluxc.persistence.WCAndroidDatabase/15.json

  "database": {
    "version": 15,
-    "identityHash": "c1b4a5003f85e61f9427f4fa0edea3e1",
+    "identityHash": "c863efcff0193dc66c6a1915f5291d07",


❗ We shouldn't change previous (existing) database schemas. I think we have to revert changes in this file (or I'm missing something).

wzieba · 2022-06-02T09:04:58Z

Oh so sorry @atorresveiga - after the review I realized that this PR is a draft 🤦 . Sorry if it wasn't meant for the review yet. I hope that my comments will be helpful either way.

wzieba

Thanks for improvements @atorresveiga !

As I'm leaving AFK today, I've added a few more comments 😅 .

wzieba · 2022-06-02T15:23:14Z

plugins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/model/OrderEntity.kt

+    @ColumnInfo(name = "isEditable", defaultValue = "true")
+    val isEditable: Boolean = true


I think we shouldn't let our data layer provide new information about the fetched object. I know we already do this for all other fields in this entity, but I think it's incorrect (I believe we discussed about this at some point with @0nko but unfortunately I didn't prepare an issue/fix).

I'd vote to make isEditable nullable. If this value will be a null in the core, it should also be a null in our app database. I think we should interpret this in a different layer. WDYT?

I think I understand your point here. However, I believe it is the data layer's responsibility to transform and prepare the data to be ready to be used by the business logic.
With that in mind, taking a closer look at our example here with the is_editable field, the only reason I see why this field could be null is that the merchant is using an older version of the API. I believe it doesn't make sense to our business to have an isEditable = null in our Order. Maybe I'm missing something, but I think that will only add an extra layer of complexity to our business logic, managing a null value that won't make any sense to have, at least for now.
I will vote to keep this value as a Boolean and keep the responsibility of preparing the data and deciding what to do when the value is null here in the data layer. That said, if the team agrees to follow the pattern you commented on above, I would be happy to change the code. WDYT?

wzieba · 2022-06-02T15:24:06Z

...ce/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/order/OrderDtoMapper.kt

                    metaData = this.meta_data.toString(),
-                    paymentUrl = this.payment_url ?: ""
+                    paymentUrl = this.payment_url ?: "",
+                    isEditable = this.is_editable ?: (this.status in EDITABLE_STATUSES)


It'd be awesome to add unit tests for this part (it's easy to miss something like here: c43c7cc

wzieba · 2022-06-02T15:25:02Z

...woocommerce/src/main/kotlin/org/wordpress/android/fluxc/persistence/migrations/Migrations.kt

 @DeleteTable(tableName = "ProductCategories")
 internal class AutoMigration14to15 : AutoMigrationSpec
+
+@DeleteTable(tableName = "OrderEntity")


This will remove the whole table - I think we want to provide a migration (I'm not sure if "auto" one) that will remove only content of the orders.

Yes, indeed. I realized this after I tested. I believed this was the same as DELETE FROM OrderEntity, but I couldn't be more wrong. Now I added a Migration to drop and recreate the table.

atorresveiga · 2022-06-03T12:11:43Z

Thanks for the review, @wzieba, it helps me correct some issues early. I'm still adding some tests. I will let you know when the PR is ready

atorresveiga · 2022-06-03T20:56:42Z

@wzieba thanks for your work here. This PR is ready for another round

nbradbury · 2022-06-06T16:39:23Z

@atorresveiga I see @wzieba has already reviewed this so it's probably good to go, but I'm curious why an auto-migration wasn't used?

atorresveiga · 2022-06-07T14:45:13Z

@atorresveiga I see @wzieba has already reviewed this, so it's probably good to go, but I'm curious why an auto-migration wasn't used?

Hi @nbradbury! Auto-migration will keep all the data from the OrderEntity table and add the new isEditable field with the default value. This could end on some Orders marked as editable but in a status that doesn't allow the Order to be changed. What I tried with this migration was to clear all the OrdersEntity data from the DB to force the app to request this info again from the API with the new isEditable field and use the API as the source of truth.

nbradbury

Thanks for the explanation about not using auto-migrations, makes sense!

Alejo added 3 commits June 1, 2022 12:22

update database table

45c1086

fallback to destructive migration

1a519e9

add isEditable field with fallback for older stores

c5d62ec

atorresveiga added [Store] WooCommerce [Store] Woo: Order labels Jun 1, 2022

atorresveiga requested review from nbradbury and wzieba June 1, 2022 18:51

atorresveiga commented Jun 1, 2022

View reviewed changes

atorresveiga mentioned this pull request Jun 1, 2022

Add is editable field woocommerce/woocommerce-android#6684

Merged

1 task

add is_editable field

b1b4181

wzieba reviewed Jun 2, 2022

View reviewed changes

Alejo added 6 commits June 2, 2022 09:28

fix isEditable initialization

c43c7cc

revert schema changes

e5d4e40

defaultValue matches default constructor value

b4df320

add AutoMigrationSpec from 15 to 16

519a7c6

address ktlint warning

bd47520

select correct migration

dbf4516

wzieba reviewed Jun 2, 2022

View reviewed changes

update the migration query

4039842

Alejo added 2 commits June 3, 2022 13:59

add tests for is_editable processing & fallback

d169395

create table only if it doesn't exist

b421af6

atorresveiga marked this pull request as ready for review June 3, 2022 20:46

atorresveiga mentioned this pull request Jun 4, 2022

Add is editable field woocommerce/woocommerce-android#6700

Merged

1 task

nbradbury self-assigned this Jun 6, 2022

nbradbury approved these changes Jun 7, 2022

View reviewed changes

nbradbury merged commit ae5410d into trunk Jun 7, 2022

nbradbury deleted the issue/6649-add-is-editable-API-field branch June 7, 2022 16:48

AliSoftware mentioned this pull request Jun 14, 2022

Merge release/20.1 after code freeze wordpress-mobile/WordPress-Android#16741

Merged

7 tasks

		@ColumnInfo(name = "isEditable", defaultValue = "true")
		val isEditable: Boolean = false

		@ColumnInfo(name = "isEditable", defaultValue = "true")
		val isEditable: Boolean = true

Add is editable api field #2435

Add is editable api field #2435

Uh oh!

Conversation

atorresveiga commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peril-wordpress-mobile bot commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wzieba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wzieba commented Jun 2, 2022

Uh oh!

wzieba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atorresveiga Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atorresveiga commented Jun 3, 2022

Uh oh!

atorresveiga commented Jun 3, 2022

Uh oh!

nbradbury commented Jun 6, 2022

Uh oh!

atorresveiga commented Jun 7, 2022

Uh oh!

nbradbury left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

atorresveiga commented Jun 1, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Jun 1, 2022 •

edited

Loading

atorresveiga Jun 3, 2022 •

edited

Loading