Skip to content

Conversation

@bugclerk
Copy link
Contributor

@bugclerk bugclerk commented Dec 18, 2025

Testing: see ticket.
As a solution - we allow custom values to be set but checking if it actually exists before saving to avoid API call errors.

Preview:

Screen.Recording.2025-12-16.at.10.57.53.mov

Original PR: #12991

UPD:

Components Created (4 total)

  • ✅ ix-user-combobox - Single user selection
  • ✅ ix-group-combobox - Single group selection
  • ✅ ix-user-chips - Multiple user selection
  • ✅ ix-group-chips - Multiple group selection

Forms Updated (10 forms) ✅

ACL & Permissions (4 forms):

  1. ✅ edit-posix-ace.component - User/group comboboxes
  2. ✅ edit-nfs-ace.component - User/group comboboxes
  3. ✅ dataset-acl-editor.component - Owner/ownerGroup comboboxes
  4. ✅ dataset-trivial-permissions.component - Owner/ownerGroup comboboxes

Other Forms (6 forms):
5. ✅ nfs-form.component - maproot/mapall user/group (4 fields)
6. ✅ cron-form.component - Run as user
7. ✅ rsync-task-form.component - User field
8. ✅ service-smb.component - Guest account & admin group
9. ✅ dataset-quota-add-form.component - User/group chips
10. ✅ privilege-form.component - DS groups (uses ix-group-chips)

Forms Correctly NOT Updated (3 forms) ✅

These require custom provider configurations:

  1. ✅ smb-acl.component
    - Custom: valueField: 'uid'/'gid', queryType: ComboboxQueryType.Smb
    - Reason: SMB-specific ID mapping
  2. ✅ map-user-group-ids-dialog/new-mapping-form.component
    - Custom: valueField: 'id'
    - Reason: Maps to numeric IDs, not names
  3. ✅ additional-details-section.component (User form)
    - Custom: valueField: 'id'
    - Reason: Group selection uses numeric IDs

(cherry picked from commit 0d8502e)
- Move debounceTime before API calls using switchMap to prevent race conditions
- Move validator attachment to ngAfterViewInit for consistency with existing pattern
- Prevents unnecessary API calls on every keystroke

(cherry picked from commit 3a55c77)
@bugclerk
Copy link
Contributor Author

Copy link
Contributor

@AlexKarpov98 AlexKarpov98 left a comment

Choose a reason for hiding this comment

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

ported.

@codecov
Copy link

codecov bot commented Dec 18, 2025

Codecov Report

❌ Patch coverage is 83.05085% with 40 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (stable/goldeye@f6e925c). Learn more about missing BASE report.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ponents/ix-group-chips/ix-group-chips.component.ts 77.14% 8 Missing ⚠️
...s/ix-group-combobox/ix-group-combobox.component.ts 75.75% 8 Missing ⚠️
...omponents/ix-user-chips/ix-user-chips.component.ts 77.14% 8 Missing ⚠️
...nts/ix-user-combobox/ix-user-combobox.component.ts 75.75% 8 Missing ⚠️
...idators/user-group-existence-validation.service.ts 87.69% 8 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##             stable/goldeye   #13009   +/-   ##
=================================================
  Coverage                  ?   86.14%           
=================================================
  Files                     ?     1831           
  Lines                     ?    67427           
  Branches                  ?     8066           
=================================================
  Hits                      ?    58088           
  Misses                    ?     9339           
  Partials                  ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@william-gr william-gr left a comment

Choose a reason for hiding this comment

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

The ticket was about audit but this needs to be true for any user/group input that works with directory services.

e.g.

Screen.Recording.2025-12-18.at.10.24.28.AM.mov

@AlexKarpov98
Copy link
Contributor

@william-gr ah, thanks for a good catch!
I'll implement it here and then port to master as well :)

@AlexKarpov98
Copy link
Contributor

@william-gr please check it out now.

@william-gr
Copy link
Member

Do we have to do this for every field/form?

This repeats often enough that would make sense to have a user/group field that we can re-use everywhere and avoid code duplication and mistakes. Am I wrong?

@AlexKarpov98
Copy link
Contributor

@william-gr I already included all required forms.
No code duplication - we have a shared validator, that's all we need.
Creating one more field won't make since we only care about the validation which is already extracted and code is shared :)

@william-gr
Copy link
Member

@william-gr I already included all required forms. No code duplication - we have a shared validator, that's all we need. Creating one more field won't make since we only care about the validation which is already extracted and code is shared :)

My point is, you still have to include the service and set up the validators on every form? What if you just forget the validator? It is a special field type, user and group that behave in a certain way, why not make it so. What am I missing?

@AlexKarpov98
Copy link
Contributor

@william-gr I think it's a simple component duplication with some extra logic. I don't like that approach.
I think we have some similar situations in forms where we could create a custom component but for me it's redundant.
Why do you think creating a custom component (a copy with extra logic) is a better solution?

@william-gr
Copy link
Member

william-gr commented Dec 19, 2025

@william-gr I think it's a simple component duplication with some extra logic. I don't like that approach. I think we have some similar situations in forms where we could create a custom component but for me it's redundant. Why do you think creating a custom component (a copy with extra logic) is a better solution?

The whole mess with user/group and DS explains why I think it could be a better solution.
These fields should have behaved that way since day one but we are still struggling to maintain them uniformly through the entire UI. I can very easily see new fields being added in the future where we will forget the validation service. I can also see possible changes in these that will require changes in all forms, we will miss them.

Case in point, it seems we are still missing the plethora of others forms to behave the same way as these. For instance I dont see changes to the ACL and permissions forms.

@AlexKarpov98
Copy link
Contributor

AlexKarpov98 commented Dec 23, 2025

@william-gr another issue - we have combobox and chips in some places.

do you want to create:
ix-user-chips/ix-group-chips
&
ix-user-combobox/ix-group-combobox

in this case? 🤔

@AlexKarpov98
Copy link
Contributor

@william-gr could you check it closely/carefully please? 🙌

* ></ix-group-chips>
* ```
*/
@UntilDestroy()
Copy link
Member

Choose a reason for hiding this comment

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

If think we are moving away from UntilDestroy

Copy link
Member

@william-gr william-gr left a comment

Choose a reason for hiding this comment

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

Found some issues:

https://github.com/user-attachments/assets/ef927fc1-492a-40c8-a15d-8b122186649b
No validation is done when you click out of the input with your mouse when there is a popup with some entries.

https://github.com/user-attachments/assets/0fbe6712-b2ee-45e0-ab05-71aaca8b335e
Cant input a custom user/group. This was in ACL edit but seems to happen everywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants