ProtonMail / WebClients

Monorepo hosting the proton web clients
GNU General Public License v3.0
4.53k stars 571 forks source link

Fix deleting duplicated contacts after merging #261

Closed cmolina closed 3 years ago

cmolina commented 3 years ago

Hello; this is a follow-up PR related to #258.

When merging duplicated contacts, the merge succeeded, but it fails to delete the duplicated contacts.

As a user, I see a progress bar that never reaches 100%.

https://user-images.githubusercontent.com/1903086/143919160-a0eda4c1-6db0-45dc-a8dd-c272b3c87d39.mov

This is happening due to an error trying to obtain the contactId from an EncyptedContact object, as the object doesn't have the mentioned property. The root cause of this issue is misleading casts (it happens twice in MergingModalContent.tsx).

beSubmittedContacts.push({ contact: encryptedMergedContact } as EncryptedContact);

I did not modify these sentences since they require some extra work refactoring some other places affected by this. Instead, I just fixed the issue by getting the contactId information directly from responses.

https://user-images.githubusercontent.com/1903086/143919257-652782df-5587-43e1-b207-52928b7b8fa6.mov

econdepe commented 3 years ago

Hey @cmolina , indeed merging contacts is broken at the moment. We have merged a fix for that, but has not made it to production yet (it should be there either later this week or the following, I'll ping you here when it's done; by the way, your #258 PR has been merged as well :), but it takes some time to propagate to production). Your proposal would work for most of the cases, except in those where the response does not contain a Contact (which may happen if the API rejects contact creation). The root of the problem is the forced cast-typing you correctly point out. The fix will remove that bad cast and a few others that could be found in the MergingModalContent file. Thanks for the PR at any rate!

cmolina commented 3 years ago

Sweet. I will close this PR then. Cheers!

econdepe commented 2 years ago

Here it is the fix I mentioned above: https://github.com/ProtonMail/WebClients/commit/9dcaaca5eac1d0901062fc6d44418536e97037be