Need help using setRelations correctly

Problem is in the inProgressEvents using a OR clause. The OR clause out of parenthesis will override every other where clause.

Change that relation to:

public function inProgressEvents()
{
    return $this->hasMany(Event::class)
        ->where(function ($query) {
            $query->whereDate('closingDate', '>=', Carbon::today())
                ->orWhereNull('closingDate');
        })
        ->orderBy('created_at', 'ASC');
}

So parenthesis (round brackets) are added around the where clauses that check the dates.

One other idea. As you will have multiple paginators on the same view, I would have different page name for each paginator.

For example, let's say the page show Organizer A and B with pagination available for their past events.

Right now if user clicks page 2 on one paginator, all paginators in the view will also change to page 2, as they all look for the same query parameter.

Using different page names will allow users to use the paginators independently.

The problem is that you'll need to compute the paginator names for each organizer to append to each dynamic generator.

I made proof of concept to illustrate it:

$user = auth()->user();

$organizers = $user->organizers()->get();

// compute current paginators for each organizer
$pageParameters = $organizers->flatMap(function (Organizer $organizer) {
    $id = $organizer->getKey();

    $pastPageKey = "past_{$id}_page";
    $currentPageKey = "current_{$id}_page";

    return [
        $pastPageKey => request()->query($pastPageKey, 1),
        $currentPageKey => request()->query($currentPageKey, 1),
    ];
})->toArray();

// compute paginators for each organizer and append
// the calculated page names
foreach ($organizers as $organizer) {
    $organizer->setRelation('pastEvents', $organizer->pastEvents()
        ->where('user_id', $user->getKey())
        // set current paginator name for past events
        ->paginate(1, ['*'], 'past_' . $organizer->getKey() . '_page')
        // add the other page names to this paginator's links
        ->appends($pageParameters));

    $organizer->setRelation('inProgressEvents', $organizer->inProgressEvents()
        ->where('user_id', $user->getKey())
        // set current paginator name for current events
        ->paginate(1, ['*'], 'current_' . $organizer->getKey() . '_page')
        // add the other page names to this paginator's links
        ->appends($pageParameters));
}

return view('my-view-path', ['organizers' => $organizers]);

Hope it is clear, and that it helps somehow.

EDIT fixes typo in current page name

Hi @rodrigo.pedra This is so amazing! I am marking the first answer as correct as it worked. However I am starting to look into your extra write up. I was planning on just having a different url route for the next or previous pages.

@rodrigo.pedra

Actually I think I am starting to understand your code more.

$pastPageKey = "past_{$id}_page";
    $currentPageKey = "past_{$id}_page";

should that be

$pastPageKey = "past_{$id}_page";
    $currentPageKey = "current_{$id}_page";

?

@chrisgrim Your code suffers from the N+1 problem. You shouldn’t be querying events in a loop like that. In fact, it’s worse than N+1, it’s N*2+1! That means, if you fetch 20 organisers, you’re going to be executing 41 queries! One to fetch organisers, and then two per organiser to fetch past and in progress events for each organiser.

@martinbean So what is the correct way to setup something like this? Should I just fetch it one time, and then just do individual fetch from then on?

Yes @chrisgrim there was a typo there, nice catch! Thanks! I fixed my code sample.

As @martinbean highlighted you have a N+1 problem, for the reasons he outlined above.

But for the setup you need, options for eager loading it properly require a bunch of work.

You want to have:

• Eager load using limits
• paginated results for each relation

Eager loading with ->limit(...) is not supported out of the box, to eager load with ->limit(...) I would consider this package:

https://github.com/staudenmeir/eloquent-eager-limit

When eager loading we loose the ease of using the ->paginate() method. If you want paginated results on eager loaded relations you'll need to:

• Take care of ->skip() and ->limit() calls when eager loading related models
• Instantiate each paginator manually after fetching the results

This is a lot of work. But if your app will have a lot of users, that can access it simultaneously I would consider that.

One option is to use something like Livewire, hydrate the first call (skipping the N+1 problem), and let it handle the pagination asyncronously with ajax.

But that also requires some manual work to get it right.

If your app is for private or internal use inside an organization, and won't have many users/events, maybe the N+1 problem won't slow it down a lot.

I can try to come up with a proof of concept, but not today as it would require a lot of work, maybe on the weekend.

In the meantime, I hope this insights give you some guidance.

And if someone else want to help you on that, that would be wonderful.

Maybe opening a fresh thread to handle this improvement is a better way to ask for more help.

I think I have come up with a pretty simple solution (I think)

In vue when I click on a pagination I have

            selectPage(page, organizer, state) {
                axios.post(`/create/${organizer.slug}/events/fetch`, {page:page, state: state})
                .then(res => {
                    console.log(res.data);
             
                });
            },

and then in laravel I have a new route that goes to my controller where I have

public function fetch(Request $request, Organizer $organizer)
    {
        if ($request->state === 'past') {
            return $organizer->pastEvents()->paginate(2);
        }
        return $organizer->inProgressEvents()->paginate(2);
    }

this just gives me information specific to that one organizer and then I will return the data to vue and overwrite the specific past event for that organizer.

Sure, using Vue for async pagination is a good solution.

Are you using it for the subsequent pagination calls? Or are you already using it in the first load?

In the first load it would still send a lot of requests to the server to fetch the initial page for each event list.

Oh I am definitely using your method for the first call to get all the organizers and events and then just the async for each following paginated call

Ok, but as @martinbean pointed out my method, will lead to a N+1 problem, as for each foreach loop iteration it makes two more databases calls.

To address that follow these steps:

1 - Install this package:

https://github.com/staudenmeir/eloquent-eager-limit

composer require staudenmeir/eloquent-eager-limit:"^1.0"

2 - As per this package readme, add this trait on both Organizer and Event models:

class Organizer extends Model
{
    use HasEagerLimit;

    // ... other code
}

class Event extends Model
{
    use HasEagerLimit;

    // ... other code
}

3- change your code to do eager load them:

    $user = auth()->user();

    $organizers = $user->organizers()
        ->with([
            'pastEvents' => function ($relation) use ($user) {
                $relation->where('user_id', $user->getKey());
                $relation->limit(5); // 4 + 1
            },
            'inProgressEvents' => function ($relation) use ($user) {
                $relation->where('user_id', $user->getKey());
                $relation->limit(11); // 10 + 1
            },
        ])
        ->get();


return view('my-view-path', ['organizers' => $organizers]);

Now all data will be loaded with 4 queries, no matter how many organizers a user have.

Note I added an extra element to the limit clauses, that will help you if you need create custom paginators as you will be able to assess if there is "more pages".

Also note no paginator is being created, so a code like this:

{{ $organizer->pastEvents->links() }}

Won't work anymore.

You should either use Vue to generate the pagination links, or loop through the results and manually create a paginator instance for each result.

Let me know if this helps.

Ahh I see, This is super helpful! Thanks so much to both of you @rodrigo.pedra and @martinbean

You're welcome! Have a nice day =)