Flagsmith / flagsmith

Open Source Feature Flagging and Remote Config Service. Host on-prem or use our hosted version at https://flagsmith.com/
https://flagsmith.com/
BSD 3-Clause "New" or "Revised" License
4.77k stars 365 forks source link

feat: Add UI for SAML attribute mapping #4184

Closed novakzaballa closed 3 months ago

novakzaballa commented 3 months ago

Thanks for submitting a PR! Please check the boxes below:

Changes

How did you test this code?

  1. Go to Organisation settings -> SAML Tab
  2. Create or select a SAML configuration
  3. Go to the Attribute Mapping tab and create a new Attribute
vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 24, 2024 2:36pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 24, 2024 2:36pm
1 Ignored Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **docs** | ⬜️ Ignored ([Inspect](https://vercel.com/flagsmith/docs/2L9b6PVPAbBthMSHtcvCq3hvzuSH)) | [Visit Preview](https://docs-git-feat-add-ui-for-saml-attribute-mapping-flagsmith.vercel.app) | | Jun 24, 2024 2:36pm |
github-actions[bot] commented 3 months ago

Uffizzi Preview deployment-53181 was deleted.

matthewelwell commented 3 months ago

@novakzaballa I've released the SAML fix that you created to staging so this can now be tested correctly against staging, but I've found a few issues:

  1. It should be possible to add attributes as part of the creation workflow (or at least the modal shouldn't auto close)
  2. The FE is making an invalid request initially when opening the edit modal to retrieve the mappings for saml_configuration=0 (see screenshot 1 below)
  3. The UI doesn't handle long IdP attributes (see screenshot 2 below - note that this is a very valid attribute and is used in a lot of our customer's configurations)
  4. The attributes aren't editable but appear like they are (as it shows a pointer when you hover over each row in the table). I guess making them editable would be tricky given the fact that we're already in a modal, so perhaps we should just remove the pointer on hover?
image image
novakzaballa commented 3 months ago
  1. It should be possible to add attributes as part of the creation workflow (or at least the modal shouldn't auto close)

Now the modal does not close when the configuration is created, and the SAML attribute tab displays immediately.

  1. The FE is making an invalid request initially when opening the edit modal to retrieve the mappings for saml_configuration=0 (see screenshot 1 below)

Corrected

  1. The UI doesn't handle long IdP attributes (see screenshot 2 below - note that this is a very valid attribute and is used in a lot of our customer's configurations)

Done

Screenshot 2024-06-20 at 3 57 44 PM
  1. The attributes aren't editable but appear like they are (as it shows a pointer when you hover over each row in the table). I guess making them editable would be tricky given the fact that we're already in a modal, so perhaps we should just remove the pointer on hover?

Removed.

matthewelwell commented 3 months ago

The overflow issue still isn't resolved. The key thing we need to solve it for is not multiple words, but a single long URL. The test case you used is not really valid.

image
matthewelwell commented 3 months ago

I've fixed this here but I'm seeing weird behaviour on the tooltip. It looks like the container for the tooltip itself isn't wide enough for the string but I have no idea how to resolve that...

image
novakzaballa commented 3 months ago

I've fixed this but I'm seeing weird behaviour on the tooltip. It looks like the container for the tooltip itself isn't wide enough for the string but I have no idea how to resolve that...

Screenshot 2024-06-24 at 10 34 43 AM

Done