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

WebUI: services without canonical name are shown correctly #166

Closed
wants to merge 1 commit into from

Conversation

pvomacka
Copy link

@pvomacka pvomacka commented Oct 17, 2016

There is a change introduced in 4.4 that new services have canonical name. The old ones
didn't have it, therefore these services were not correctly displayed in WebUI.

This patch adds support for this type of services. Service name is taken from
'krbprincipalname' attribute in case that 'krbcanonicalname' attribute is not present
in server response.

https://fedorahosted.org/freeipa/ticket/6397

@pvoborni
Copy link
Member

Works for me. Code look OK. I have only one minor nitpick: name of the adaper. Current is SearchTableColumnFieldAdapter imho better would be e.g. AlternateAttrFieldAdapter. I.e., to describe function and not place of use. IMO it can be even placed in field.js

If you don't want to address that then ACK.

@pvomacka
Copy link
Author

Thank you for review. I moved the adapter into field.js and also renamed it. Proposed name looks better.

There is a change introduced in 4.4 that new services have canonical name. The old ones
didn't have it, therefore these services were not correctly displayed in WebUI.

This patch adds support for this type of services. Service name is taken from
'krbprincipalname' attribute in case that 'krbcanonicalname' attribute is not present
in server response.

https://fedorahosted.org/freeipa/ticket/6397
@pvomacka
Copy link
Author

I forgot to improve AlternateAttrFieldAdapter comment. Fixed now.

@pvoborni pvoborni added the ack Pull Request approved, can be merged label Oct 31, 2016
@pvoborni
Copy link
Member

new version works for me ACK

@pvoborni pvoborni added the pushed Pull Request has already been pushed label Oct 31, 2016
@pvoborni pvoborni closed this Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants