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

Filter event controller index results #481

Merged

Conversation

bcobo341
Copy link
Contributor

@bcobo341 bcobo341 commented Mar 8, 2022

Description

Closes #437

  • Filter event index

@m-triassi m-triassi changed the base branch from main to feat-event-management March 8, 2022 20:24
Copy link
Contributor

@m-triassi m-triassi left a comment

Choose a reason for hiding this comment

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

So there was an issue with recursive relations loading, i fixed that for you, but while looking at this i noticed that this doesn't capture the exact filtering criteria properly as well

@@ -16,8 +16,11 @@ class EventController extends Controller
*/
public function index(Request $request)
{
$events = Event::all();
$events = Event::whereRegistered()
Copy link
Contributor

Choose a reason for hiding this comment

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

if you have this here, it means that we're always filtering the events by if a user is registered or not. What you'd want to do is check for a flag like $request->has('registered_only') or what ever name you want in an if statement, if it has that then you add the whereRegistered() call, otherwise don't add it

@bcobo341 bcobo341 added back end enhancement New feature or request task This issue represents a task belonging to a story labels Mar 9, 2022
@bcobo341 bcobo341 marked this pull request as ready for review March 9, 2022 08:38
Copy link
Contributor

@m-triassi m-triassi left a comment

Choose a reason for hiding this comment

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

Couple comments for ya!

app/Http/Controllers/EventController.php Outdated Show resolved Hide resolved
$response = $this->actingAs($user)->get(route('event.index', [
'search' => $event->title,
'registered_only' => true,
'registered_user' => $user->id,
Copy link
Contributor

Choose a reason for hiding this comment

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

what about a test for if registered_user is null? i suspect it likely will be null since 99% of the time we're filtering this list for the logged in user, which whereRegistered() does automatically

@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@bcobo341 bcobo341 requested a review from m-triassi March 10, 2022 05:50
Copy link
Contributor

@m-triassi m-triassi left a comment

Choose a reason for hiding this comment

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

Seems about right to me!

@m-triassi m-triassi merged commit 508e9f9 into feat-event-management Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back end enhancement New feature or request task This issue represents a task belonging to a story
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter EventController index results
2 participants