Add users filtering while exporting #362
Conversation
|
@llaske please review it and lemme know if any changes are required. |
|
Few warnings are coming in console. Should I define them in locale.ini file?
|
|
@codebloded can you review this once, and let me know if any changes are required? |
|
yes ! |
| function downloadUsers(event) { | ||
| $.ajax({ | ||
| type: "GET", | ||
| url: "/dashboard/users/export", | ||
| url: `/dashboard/users/export/${roleValue}/${usernameValue}/${classroomValue}`, |
There was a problem hiding this comment.
Instead of making these parameters part of the URL, it'll be better group them in a query.
There was a problem hiding this comment.
Sure, that makes more sense.
| var filteredUsers = []; | ||
| var requiredIDs = new Set(); | ||
| if(selectedClassroom !== 'undefined'){ | ||
| selectedClassroom.split(',').forEach(id => { | ||
| requiredIDs.add(id); | ||
| }); | ||
| } | ||
| users.forEach(user => { | ||
| let required = true; | ||
| if (selectedUsername !== 'undefined') { | ||
| if (user.name !== selectedUsername) { | ||
| required = false; | ||
| } | ||
| } | ||
| if (required && selectedRole !== 'all' && user.type !== selectedRole) { | ||
| required = false; | ||
| } | ||
| if (required && selectedRole === 'student' && selectedClassroom !== 'undefined') { | ||
| if (!requiredIDs.has(user._id)) { | ||
| required = false; | ||
| } | ||
| } | ||
| if(required) filteredUsers.push(user); | ||
| }) |
There was a problem hiding this comment.
- Filtering logic can be simplified.
- Some variable names are misleading, like
requiredIDs. - Fix indentation.
- Maybe consider using Array.filter.
There was a problem hiding this comment.
Okay I will look into it. Thank you for the feedback!
|
I have been a little busy, so missed on completing this pull request. I will look into the changes. Thanks for the wait! |
|
@mayank190801 sounds like there a lint issue. Could you fix it? |
|
I have created another pull request #404 regarding this issue. We can continue our discussion there. Thanks. |
Fixes issue #313
I have taken motivation from last pull request on this issue.