Add attribute mapping config #192
Conversation
pablothedude
force-pushed
the
feature/add-attribute-mapping
branch
2 times, most recently
from
9789f2a to
fc3c617
Compare
There was a problem hiding this comment.
All in all this looks good! Several smaller issues should be addressed. See individual feedback entries in the code review for that.
Also, why not utilize the attribute dictionary to verify if the configured attributes actually make sense. This could be done in a DI extension. Or alternatively in the constructor of the mapping helper.
src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/RemoteVettingController.php
Show resolved
Hide resolved
src/Surfnet/StepupSelfService/SelfServiceBundle/Service/RemoteVetting/AttributeMapper.php
Outdated
Show resolved
Hide resolved
| {% block ss_remote_vet_assertion_widget %} | ||
| {% spaceless %} | ||
| <div class="text_widget"> | ||
| <h3>{{ form.vars.data.name }}: '{{ form.vars.data.value|join(', ') }}'</h3> |
There was a problem hiding this comment.
I love that you made this into a widget, that way we have a lot of control over the rendering! Some remakrs/questions/general feedback.
- I'm not sure from a semantics standpoint, using a h3 heading here is best? I'm by no means an expert on this topic. But looking at it as a visitor. The large headings are somewhat overpowering. Putting this data into a table might make things much more clear. But that might prove difficult?
- Also, more importantly, please make the
form.vars.data.nametranslatable. Or somehow yield the translations from the saml-bundle. Users might not know the meaning of an attribute identifier. - Finally, please remove the (optional) portion of the labels. They make for messy user experience. Especially the ones that fall behind the textareas.
Let me be clear. For a POC this is probably good enough!
There was a problem hiding this comment.
Because this is a POC I didn't want to take too much effort and didn't added styling whatsoever. I think it's up to the PO first if the form has all fields required. If that's clear we could update the styling. But I certainly agree some additional TLC will be needed.
There was a problem hiding this comment.
Maybe still look into points 2 and 3. Especially point 3 would look silly even in a POC demo.
There was a problem hiding this comment.
Regarding point 2 and 3. I'll create a Pivotal issue for this and address this then so this 'll become none blocking.
...fnet/StepupSelfService/SelfServiceBundle/Tests/Service/RemoteVetting/AttributeMapperTest.php
Outdated
Show resolved
Hide resolved
...urfnet/StepupSelfService/SelfServiceBundle/Service/RemoteVetting/IdentityProviderFactory.php
Outdated
Show resolved
Hide resolved
...urfnet/StepupSelfService/SelfServiceBundle/Service/RemoteVetting/IdentityProviderFactory.php
Outdated
Show resolved
Hide resolved
src/Surfnet/StepupSelfService/SelfServiceBundle/Service/RemoteVetting/RemoteVettingContext.php
Show resolved
Hide resolved
src/Surfnet/StepupSelfService/SelfServiceBundle/Service/RemoteVetting/SamlCalloutHelper.php
Outdated
Show resolved
Hide resolved
src/Surfnet/StepupSelfService/SelfServiceBundle/Service/RemoteVetting/Value/AttributeMatch.php
Show resolved
Hide resolved
pablothedude
force-pushed
the
feature/add-attribute-mapping
branch
4 times, most recently
from
ae2f47f to
c4152ae
Compare
pablothedude
force-pushed
the
feature/add-attribute-mapping
branch
from
c4152ae to
b183a1b
Compare
Add mapper to map local and external (remote vetting) attributes so they could be used to match values on. Currently only a form is filled in other to select valid attributes.
pablothedude
force-pushed
the
feature/add-attribute-mapping
branch
from
b183a1b to
991710b
Compare
Add mapper to map local and external (remote vetting) attributes so they can be used to match values on. Currently, only a form is filled in order to select valid attributes.
https://www.pivotaltracker.com/story/show/171571478