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

use prefetched user list in select user component #4647

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

petrjasek
Copy link
Member

@petrjasek petrjasek commented Sep 25, 2024

there was an issue when fetching desk members
for a desk with many users, it would fail on
too big request when fetching 50+ users by id.

SDCP-849

there was an issue when fetching desk members
for a desk with many users, it would fail on
too big request when fetching such users by id.

SDCP-849
@petrjasek petrjasek added this to the 2.8 milestone Sep 25, 2024
@petrjasek petrjasek requested a review from thecalcc September 25, 2024 11:18
@@ -11,6 +11,7 @@ import {time} from './time';
import {user} from './user';
import {vocabularies} from './vocabularies';
import {contentProfiles} from './content-profiles';
import * as utils from './utils';
Copy link
Member

Choose a reason for hiding this comment

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

why not import from lodash directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

will drop it, seems like we have another helper for it


if (searchString != null && searchString.length > 0) {
query.$and.push(getUserSearchMongoQuery(searchString));
}
const query = getUserSearchMongoQuery(searchString);
Copy link
Member

Choose a reason for hiding this comment

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

Your filtering here only supports a subset of mongo query, ($or operator). How about next to getUserSearchMongoQuery you put matchUserByMongoQuery(user: IUser): boolean so if someone changes the query they would also see client side matching code and would update it too.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@tomaskikutis
Copy link
Member

too big request when fetching 50+ users by id

You mean GET request URL would get too long?

@tomaskikutis
Copy link
Member

tomaskikutis commented Sep 25, 2024

By the way, that playwright test is unstable. You can either try restarting e2e or disable that test. (the problem is actually the feature that is unstable, not the test)

@petrjasek
Copy link
Member Author

too big request when fetching 50+ users by id

You mean GET request URL would get too long?

yes exactly

@petrjasek petrjasek enabled auto-merge (squash) September 26, 2024 10:40
@petrjasek petrjasek disabled auto-merge September 26, 2024 10:40
@petrjasek petrjasek merged commit d049442 into superdesk:develop Sep 26, 2024
2 of 3 checks passed
@petrjasek petrjasek deleted the fix-select-user-component branch September 26, 2024 10:40
petrjasek added a commit that referenced this pull request Sep 26, 2024
* use prefetched user list in select user component

there was an issue when fetching desk members
for a desk with many users, it would fail on
too big request when fetching such users by id.

SDCP-849
@tomaskikutis
Copy link
Member

I was looking around the code and realized this change is breaking SelectUser interface and probably usages. According to the interface deskId is optional and according to your code it looks like it wouldn't work without it?

@petrjasek
Copy link
Member Author

yep you're right, will fix it

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.

3 participants