Closed thabotswana closed 2 years ago
cc @ThomazFB @hichamboushaba @wzieba , this issue seems related to the OrderDAO changes. Would one of you mind taking a look this week please?
I've also noticed in sentry an issue similar to this one, but instead of null line items, it's null metadata. @wzieba should we make the nullability of the Order model fields more flexible?
Yes, as API contract is not as stable as we would wish (is that because of plugins? 🤷 ) we should make our restrictions less conservative.
I wonder why it didn't popped out before, when using WellSql. I hope to learn it in the proccess.
Linking recent discussion about nullable fields in OrderEntity
: https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/2295#discussion_r810347906
I wonder why it didn't popped out before, when using WellSql. I hope to learn it in the proccess.
Maybe it has to do with how Room will map the "null"
string vs what WellSql was doing, currently we map the API response fields using toString
in our mapper, which would result in saving the String value "null"
to the DB entity.
@hichamboushaba that's good idea but I couldn't reproduce it in any way, on different phones.
The issues from Sentry shows that this happens on Samsung phones so your theory might be very true - maybe SQL driver on some Samsung phones treats "null"
as NULL
.
And honestly, saving String with value "null"
instead of empty String is a bug in our code already I think.
Applying the same logic of ?: ""
or .orEmpty()
, like in other fields, would probably fix this issue but I'll try to reproduce the issue with help of other teammates that have Samsung phones with this tests: https://github.com/wordpress-mobile/WordPress-FluxC-Android/compare/woo/6013_add_test_cases_for_db_null_value?expand=1
And honestly, saving String with value "null" instead of empty String is a bug in our code already I think.
Yes, I agree it's a bug, and I don't think it was intentional, it was just missed in the mapper.
Just realised that this has been resolved "accidentally" by https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/2308 PR - now, the null will be mapped to empty JSON array which is even more accurate than empty String I believe (as those fields are meant to be JSONs):
As I wasn't able to reproduce the issue, I close it on Sentry (mark as resolved in 8.8
) and we'll observe if it'll return.
Edit: I'll do this ☝️ when 8.8
will be available in Sentry.
The user had asked for an update. Is there anything we can tell them?
4844740-zen
@thabotswana I'm now trying to reproduce the issue once again and check if recent changes fixed the thing. I should have some answers EOD, I'll ping you here. Is that okay?
@thabotswana unfortunately, I haven't found a way to reproduce the original issue so far. We can just hope that 8.8
release will resolve the issue. If not, we'll continue working on it.
@wzieba just to confirm, does this mean we should ask the user to wait until the 8.8
is released?
Or do y'all still want to run some testing or continue working on this?
@wzieba just to confirm, does this mean we should ask the user to wait until the 8.8 is released?
Yes, we can communicate that 8.8
is supposed to fix the bug. If they'll still report the issue, we'll work on that further.
Thanks and sorry for that uncertainty - as I couldn't reproduce issue, we can't be sure that we have the fix.
I've looked at Sentry dashboard and it seems that we successfully fixed this bug in 8.8
: https://sentry.io/organizations/a8c/discover/results/?id=13523&project=1459556&statsPeriod=90d
Closing this issue then.
A user reported that the WooAndroid app crashes when accessing orders. They say its working on iOS.
Reported in 4844740-zen.
Sentry reports found here.
Mobile Environment Please include:
WordPress Environment