-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add fill options for Phone and E-mail details #83
base: master
Are you sure you want to change the base?
Conversation
I have also added a fix for #85 which deals with the same issue but then for email. It was not really possible to create two PRs because the code would interfere with this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through the code but have not tested it. Adding a unit test or two would help document the various cases and ensure they were handled as designed.
Hi @jaapjansma and @artfulrobot, we're currently compiling a |
@bjendres it has been a while ago since I wrote this PR. So I don't know exactly what the status is. |
@bjendres I did an upgrade for a client which is using this functionality. So I checked this PR. And a rebase. Resolved merge conflicts. And also had a look at the feedback from @artfulrobot which was helpful and followed up his advice. Can you have another look and possible merge this? |
With the fill details options I would expect that a phone number is added when no phone number with the location type and phone type exists.
When a phone number with that location type and phone type already exists I would expect that nothing changes. However the new phone number gets added and I end up with two phones of the same location type and phone type.
This PR enhances the settings for fill details for phone numbers. The same issue also exists with e-mail addresses and this PR also fixes it for the e-mail addresses.
Before
Below a screenshot of the settings screen:
After
Below a screenshot with the changes applied to the settings screen:
And for e-mail
The changes are backwards compatible. Meaning that if someone had selected phone at the Fill details, the Fill Phone is set to
If contact has not this phone number
.Comment
This PR also removes the phone_numeric field from the diff activity.