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

Fix custom labels not being displayed #2064

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Fix custom labels not being displayed #2064

merged 1 commit into from
Feb 18, 2021

Conversation

j-maas
Copy link
Contributor

@j-maas j-maas commented Feb 17, 2021

Should fix #1842.

The canDisplay filtering was introduced almost a year ago. It checks whether the property name is "registered" in rfcProps.js, and if not, it does not pass it along.

The problem is that custom labels are implemented by using two vCard fields with the same, e.g., group1.X-ABLabel and group1.TEL, where the . separates the prefix from the field types. The sorting function sorts by what is after the ., since we are not interested in the prefix. By the same logic, we should only check what is after the . for whether it is a property we can display.

I don't have the energy to figure out how to test this change. But I'm confident enough that this is the fix that I wanted to propose it in the form of a PR.

@j-maas
Copy link
Contributor Author

j-maas commented Feb 17, 2021

Sorry, I had an embarrassing missing paren, so I edited the commit.

@j-maas
Copy link
Contributor Author

j-maas commented Feb 17, 2021

And now DCO is happy.

@j-maas
Copy link
Contributor Author

j-maas commented Feb 17, 2021

And now I turned of my auto-formatter. Sorry for the noise.

@skjnldsv skjnldsv added 3. to review Waiting for reviews bug Something isn't working high High priority labels Feb 18, 2021
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Hey, thanks for this!
The fix should go in the canDisplay method.

const propType = propModel && propModel.force
? propModel.force
: property.getDefaultType()
return propModel && propType !== 'unknown'
},
/**

Here you drop the normal properties. You need to stay compatible with the non-split properties

@skjnldsv
Copy link
Member

Done, I pushed! @y0hy0h please double check my changes :)

Thanks a lot for narrowing this!

Custom labels are represented as two fields with a prefix, e.g., `group1.X-ABLabel` and `group1.TEL`. Previously, `canDisplay` would check whether it could display `group1.TEL`, which is never a known property type. Instead we now check the property type after the `.`, which in our example is `TEL` which is displayable.

Signed-off-by: Y0hy0h <y0hy0h@gmx.net>
@j-maas
Copy link
Contributor Author

j-maas commented Feb 18, 2021

Yeah, this makes sense! Also great idea to add the comment.

I'm happy it helped! Thanks for fixing the fix so quickly. 😊

@skjnldsv skjnldsv merged commit 3dc0de2 into nextcloud:master Feb 18, 2021
@welcome
Copy link

welcome bot commented Feb 18, 2021

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/contacts/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-contacts and #nextcloud-dev on Freenode for a chat!

@j-maas j-maas deleted the patch-1 branch February 18, 2021 11:30
@sciurius
Copy link

I can confirm that this solved the problem for me. I have many contacts that have ADR entries similar to

item0.ADR;TYPE=HOME: ...
item0.X-ABADRLnl

With the fix the address information is displayed again.

Thanks, Johannes!

@skjnldsv
Copy link
Member

I'll release a new version as soon as possible

@PHP-Expert

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

@PHP-Expert
Copy link

#3645

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ITEM1 property not displayed
5 participants