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

User list export: apply filters (fixes #298) #481

Merged
merged 7 commits into from Apr 17, 2024

Conversation

Trigary
Copy link
Collaborator

@Trigary Trigary commented Mar 23, 2024

Summary of changes:

  • The implementation behind the export button on the /users route has been replaced
    • The rendering has been moved from list.blade.php to list_users_component.blade.php
    • Clicking the button no longer calls the users.export route: the click is now handled within the livewire component
    • The same filtering logic is used within the component for the display of the users and for the exporting
  • To implement the filtering, a UserFilter utility class has been created
  • The different Export classes under UsersSheets have been modified so that the included users can be specified
  • The behavior of the users.export path (hopefully) hasn't changed: the same users will get included in the exports as before

Small, mostly unrelated fixes:

  • The $show_feedback property has been deleted from SemesterEvaluationExport: it was unused
    • UsersExport also assumed that SemesterEvaluationExport had a boolean constructor parameter, while in fact, it did not have any parameters. This has also been fixed.
  • ListUsers's status related methods incorrectly named their parameters status_id, while in reality they received a status (string), not an ID

@Trigary Trigary requested a review from a team as a code owner March 23, 2024 22:15
app/Utils/UserFilter.php Outdated Show resolved Hide resolved
app/Http/Controllers/Secretariat/UserController.php Outdated Show resolved Hide resolved
app/Exports/UsersExport.php Outdated Show resolved Hide resolved
app/Exports/UsersExport.php Outdated Show resolved Hide resolved
app/Models/User.php Outdated Show resolved Hide resolved
app/Models/User.php Outdated Show resolved Hide resolved
app/Models/User.php Outdated Show resolved Hide resolved
@Trigary
Copy link
Collaborator Author

Trigary commented Apr 4, 2024

The new scopeWithAllRoles doesn't (yet) support specifying role objects and the new tests don't use role objects. Let me know if I should change that.

@Trigary Trigary requested a review from kdmnk April 4, 2024 13:37
@kdmnk
Copy link
Contributor

kdmnk commented Apr 4, 2024

The new scopeWithAllRoles doesn't (yet) support specifying role objects and the new tests don't use role objects. Let me know if I should change that.

Yeah I was also thinking about that but honestly I kind of want to rewrite the whole scope to support multiple roles in an optimal way and then use the scope for the hasRole function.

Whether to support object arguments or IDs is also a debatable. Objects improve readability and static typing whereas IDs are usually more optimal (if you only have the id you should be able to pass it as it is).

In short, it's fine by me, but feel free to investigate it further.

I'll take a look at this whereHas use case later, seems strange.

@kdmnk
Copy link
Contributor

kdmnk commented Apr 4, 2024

FYI you can use https://laravel.com/docs/11.x/queries#debugging to see make sure the queries are good. Or also in laravel debugbar.

Copy link
Contributor

@kdmnk kdmnk left a comment

Choose a reason for hiding this comment

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

Checked the docs. LGTM, if you want to merge now.

@Trigary
Copy link
Collaborator Author

Trigary commented Apr 16, 2024

I want to merge now.

@horcsinbalint horcsinbalint merged commit c1b584c into EotvosCollegium:development Apr 17, 2024
5 checks passed
@Trigary Trigary deleted the issue-298 branch April 17, 2024 20:48
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.

None yet

3 participants