search

wordpress-mobile / WordPress-FluxC-Android

WordPress Network and Persistence layer based on the Flux architecture
GNU General Public License v2.0
58 stars 37 forks source link

Update the site duplication logic to not throw when IDC occurs #3066

Closed hichamboushaba closed 3 months ago

hichamboushaba commented 4 months ago

Issue

We have a logic that detects duplicate sites when inserting them to the database, this logic is causing an issue in the WCAndroid app where users with duplicate websites under their WordPress.com account can't login to them, the duplication of WordPress.com sites can happen during an "Identity Crisis" (IDC) (PCYsg-Jfw-p2).

This PR fixes the issue by relaxing the condition to allow inserting duplicate websites coming from the WPCom API to allow the login in the client apps.

Rationale of the change of the PR

Looking at the historic of the logic mentioned above, I believe it was added due to special circumstances in the WordPress app to deal with duplication that could happen when an XMLRPC website gets added later as Jetpack website or the other way around, but for some reason it also checks for duplicates when both websites are coming from the WordPress.com API, and I didn't find an explanation of why this was added. This logic to prevent adding duplicates is not available in any other client (as far I was able to check), the login works to those duplicated websites work well with Calypso and the iOS apps:

Calypso Jetpack iOS app Woo iOS app
IMG_0115 IMG_0116

Given this, I think the Android apps should behave the same, as the backend allows those duplicate websites for a reason (to preserve the user's data after the IDC), we shouldn't lock the users out of signing in to those websites.

Testing

Example app

  1. DM and I'll share with you the credentials of an account with IDC.
  2. Login using the example app.
  3. Go to the Sites screen, and tap on Log Sites.
  4. Confirm the site with IDC is listed.

WCAndroid

https://github.com/woocommerce/woocommerce-android/pull/12143

hichamboushaba commented 4 months ago

@aforcier before moving forward with this PR, can I get your opinion on the changes and the explanation I wrote above in the description? does it make sense for you or am I missing some cases here?

Sorry for pinging you directly, I saw you edited some parts of this logic in the past, and I know it's been a long time ago, but if you still remember something about the reasons, it would be really helpful to have more confidence in the suggested change.

hichamboushaba commented 3 months ago

@nbradbury sorry for pinging you multiple times these days, this is one additional PR to fix an issue with the login, please read the PR description, and tell me if it makes sense to you. IMO, there is no need to update FluxC right now in WPAndroid; the changes should be transparent and will be brought in using other FluxC updates, but please let me know if you prefer to have an explicit PR in WPAndroid and I can create it.

JorgeMucientes commented 3 months ago

The rationale on the changes makes sense to me and the code changes look good and work as expected. Nice job!