Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto merge #2764

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Auto merge #2764

wants to merge 3 commits into from

Conversation

MrPompom
Copy link
Contributor

@MrPompom MrPompom commented Jun 2, 2022

Linked to the pull request #2756 (comment)

Allow to automatically merge contacts who have the same Fullname, Phone number or Email adress

How it works

When the auto merge button is clicked, all the contacts which have the same Fullname, phone number or email adress will be merged together

image

Add checkbox on hover and add a button to delete the selected contacts

Signed-off-by: Bastien PROMPSY <bastienprompsy@gmail.com>
Add a button to merge the selected contacts

Signed-off-by: Bastien PROMPSY <bastienprompsy@gmail.com>
Add a button auto merge which will merge the possible duplicated contacts
verified by the fullname, phone number and email adress

Signed-off-by: Bastien PROMPSY <bastienprompsy@gmail.com>
@jancborchardt
Copy link
Member

Nice work @MrPompom! :)

To prevent mistakes, what do you think of doing it in a way similar to other Contacts apps which have a "Merge duplicates" entry if there are duplicates?

Then people can go through the suspected duplicates and double check, instead of possibly merging some correct and some wrong.

@ChristophWurst
Copy link
Member

who have the same Fullname, Phone number or Email adress

I'm not able to read this from your code, so is this condition an or or an and? In larger organizations there are often people with the same full name. E.g. we had customer issues about this when the share menu wasn't distinguishable and now it also shows the email address. So it's important that the match is based on more than just the fullname.

@ChristophWurst ChristophWurst added enhancement New feature or request 3. to review Waiting for reviews labels Sep 14, 2022
@st3iny
Copy link
Member

st3iny commented Oct 11, 2022

First of all, thanks for your contribution.

I like the idea but I'm concerned with the UX. Automatically merging contacts involves too much magic. IMO, we should implement some form of dialog/preview similar to what Jan said.

@MWL1701
Copy link

MWL1701 commented Oct 17, 2022

Hi,
I'll add something to look over ... maybe this is helpful to determine duplicates:

I might show something, what could help to identify duplicates ...
Maybe you know a software called gSyncIt (https://www.fieldstonsoftware.com/software/gsyncit5/?gclid=CjwKCAjw-rOaBhA9EiwAUkLV4ncJIelfTRjuFwlpUvZZsE461VARySjZRqJ7x9wqoIzWwdVNHeMNMRoCbeIQAvD_BwE

This software contains a functionality to determine duplicates and "delete" them (move them to the recycle bin).
Of course we don't want a delete, but I would appreciate something like this.
In this software you can define the fields that should be compared and only if all of these criteria are identical we have a duplicate.
Something like this would be great for NextCloud-Contacts.

} else if (value[3] !== '') {
let include = false
firstContact.jCal[1].forEach(element => {
if (element[0] === value[0] && element[3] === value[3]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do the magic indeces 0 and 3 come from?

Comment on lines +274 to +278
getKey(contact, index) {
if (!this.selected.includes(contact[index].key)) {
this.selected.push(contact[index].key)
}
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method is very confusing. why doesn't it return a value? please rename or restructure to make it an actual getter

@@ -163,4 +331,8 @@ export default {
.contacts-list__header {
min-height: 48px;
}

.merge-button {
float: right;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use flexbox instead of floats

this.$store.dispatch('deleteContact', { contact: this.contacts[element] })
}
})
this.$store.dispatch('updateContact', firstContact)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateContact doesn't send the change to the server

@ChristophWurst
Copy link
Member

@MrPompom could you pleaes have a look at the feedback above?

@ChristophWurst ChristophWurst added 0. to triage Pending approval or rejection. This issue is pending approval. and removed 3. to review Waiting for reviews labels Apr 20, 2023
@ChristophWurst ChristophWurst marked this pull request as draft April 20, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. to triage Pending approval or rejection. This issue is pending approval. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants