-
Notifications
You must be signed in to change notification settings - Fork 1.2k
API: Fix pagination issue with listing LDAP configuration #11444
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
base: 4.20
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11444 +/- ##
============================================
+ Coverage 16.16% 16.20% +0.04%
- Complexity 13278 13319 +41
============================================
Files 5656 5656
Lines 497911 499237 +1326
Branches 60383 60933 +550
============================================
+ Hits 80464 80913 +449
- Misses 408492 409350 +858
- Partials 8955 8974 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@blueorangutan package |
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14625 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
@blueorangutan test keepEnv |
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-14063)
|
@pearl, It does not seem to work, I think we must consider the option ro add a uuid to the API/records |
Seemed to be a UI issue. It works via cmk. Thanks for testing @DaanHoogland |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 14641 |
@@ -21,7 +21,7 @@ | |||
:loading="loading" | |||
:columns="isOrderUpdatable() ? columns : columns.filter(x => x.dataIndex !== 'order')" | |||
:dataSource="items" | |||
:rowKey="(record, idx) => record.id || record.name || record.usageType || idx + '-' + Math.random()" | |||
:rowKey="(record, idx) => hasNoUniqueKey() ? (idx + '-' + Math.random()) : (record.id || record.name || record.usageType || idx + '-' + Math.random())" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about making the key/id be the concatenation of hostnam+”-“+port+”-“+domainid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point @DaanHoogland
also, can we create a method for this ? @Pearl1594
@Pearl1594 , the pagination works but no it reveals new issues, sorting does not work beyond the page we are looking at. I do not consider this a big issues as most installations will not have enormous amounts of ldap configurations, but still it seems like a(nother) justifications for giving ldapconfigurations a proper uuid. That said, lgtm. |
@blueorangutan test |
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian Build Failed (tid-14077) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java code lgtm
@@ -21,7 +21,7 @@ | |||
:loading="loading" | |||
:columns="isOrderUpdatable() ? columns : columns.filter(x => x.dataIndex !== 'order')" | |||
:dataSource="items" | |||
:rowKey="(record, idx) => record.id || record.name || record.usageType || idx + '-' + Math.random()" | |||
:rowKey="(record, idx) => hasNoUniqueKey() ? (idx + '-' + Math.random()) : (record.id || record.name || record.usageType || idx + '-' + Math.random())" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point @DaanHoogland
also, can we create a method for this ? @Pearl1594
Description
This PR fixes issue with pagination when listing ldap configuration. When there are more than 20 entries and we want to list the 2nd page of ldap configs when using page size 20 , it returns the 1st 20 entries always.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?