search

ProtonMail / WebClients

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

Fix merge contact preview labels #258

Closed cmolina closed 2 years ago

cmolina commented 2 years ago

Hello! This is my first PR to an open-source project :)

The purpose of this change is to address a bug that appears in ProtonMail in the merging contacts preview component, showing the emails with the wrong group (or label as appears in the code).

In my personal email I have 2 contacts called "Banco de Chile":

Expected behavior

When viewing the merging contacts preview, I should see each email with their corresponding label.

Screenshot of the preview component, showing 2 emails, each one with a different label

Current behavior

When viewing the merging contacts preview, I see each email with the wrong label, or no label at all.

Screenshot of the preview component, showing 2 emails, but only 1 label which is wrong

This is misleading, and made me believe I should not merge my contacts since I would lose information.

What is included in this CR

I realized that ContactView component is the one responsible for displaying a contact; and that this component is used 3 times across the codebase; however, in order to work correctly, it needs to receive a list with the contactEmails for obtaining the corresponding labels.

I replaced the contactEmails, which was initialized as a list with all the emails of every contact, to only have the emails from the contacts being merged.

How I tested this

Manually. I looked for unit tests, but I couldn't find any. If anyone is interested pairing with me, I'd be happy to learn how to add a new unit test for this feature.

I confirm the merge happen successfully; however the original contacts were not deleted. After this change gets approved, I would like to address that different issue in a separated PR (#261).

cmolina commented 2 years ago

Hello, I haven't heard back from you, and I like to know if there is anything I can do to get this merged soon. Thanks, Carlos

econdepe commented 2 years ago

Hi @cmolina,

Sorry for the delay answering and thanks a lot for your very well written PR. The bug you point out is indeed there, and the solution you propose looks good to me!

We still have parts of the apps uncovered by tests (of course more than what we would like; we're working on improving the test coverage), and the contact widget is one of them. We haven't fully decided yet whether to go for end-to-end or integration tests for a thing like this one. We would also like to refactor the contact data structure, but again we still haven't had time to address that. So it's easier if we leave the tests out for the moment.

I'll let you know when your PR is merged!

cmolina commented 2 years ago

Hi @econdepe, sounds great!

econdepe commented 2 years ago

Merged in https://github.com/ProtonMail/WebClients/commit/75fa13d994cf6c9110ba1a262c837c4d64ddeb93 ;) Thanks again for the PR! :pray: