I see I'm breaking a couple of tests with this change, but those tests allow something I don't think we'd prefer in practice.

As with /events, the iCal feed and homepage events will make today's events disappear the instant they start.

I feel we should show events through the end of the day they are taking place. I don't know that we'd continue to use truly instantaneous "future" events, but we could revert scopeFuture() and / or create a method like scopeOngoingAndFuture() to allow for the logic I'm suggesting with things not falling off until after an event's final day has passed.

It looks like one of the tests is failing because expire_at is nullable and the test event doesn't define an expire_at date.

We are adding 2 hours to events in the calendar and it appears from the database that we started setting an expire_at on events around June 2025.

Could we switch to requiring an expire_at and update the event scopes and tests to work off of expire_at ?

Pinging @bogdankharchenko

@oliviasculley

Thanks for the review.

I agree we should have new tests of the desired functionality. I had that in there, but then took it out while trying to debug the existing tests failing. I forgot to put it back in. I've pasted those below as an example of what I intended to add.

As for modifying the existing tests, my assessment is that they all were effectively flawed or buggy.

test_upcoming_events_sorts_unordered_events was missing the events.expire_at. We seem to set events.expire_at on the import since around mid-2025. The tests probably should reflect that events.expire_at is set.

If we accept this PR, I don't think we'd ever call the old scopeFuture() in the Events model. We might even consider removing it for that reason. Either way, test_older_events_never_show_up_on_calendar_feed would fail because this PR would, temporarily, make past events show in the feed until the end of the current day, which this PR proposes is the desirable functionality that we should expect.

test_robots_txt_disallows_all_in_non_production was failing because I have my environment set to local and we return Disallow: / when visiting /robots.txt in both testing and local environment. In practice, localhost isn't going to get indexed by search engines, so it doesn't matter what the robots.txt is for local. Since this test is called "in non production" it seems like we'd want to confirm that local and testing return the same result, which was the reason for the change.

namespace Tests\Feature\Models;

use App\Models\Event;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Tests\TestCase;

class EventTest extends TestCase
{
    use RefreshDatabase;

    public function test_scope_ongoing_and_future_includes_ongoing_events_expiring_right_now(): void
    {
        $this->freezeTime();
        $event = Event::factory()->create(['expire_at' => now()]);

        $this->assertTrue(Event::future()->get()->contains($event));
    }

    public function test_scope_ongoing_and_future_includes_events_that_expired_earlier_today(): void
    {
        $this->freezeTime();
        $event = Event::factory()->create(['expire_at' => now()->startOfDay()]);

        $this->assertTrue(Event::future()->get()->contains($event));
    }

    public function test_scope_ongoing_and_future_includes_multi_day_events_expiring_in_the_future(): void
    {
        $this->freezeTime();
        $event = Event::factory()->create(['expire_at' => now()->addDays(3)]);

        $this->assertTrue(Event::future()->get()->contains($event));
    }

    public function test_scope_ongoing_and_future_excludes_events_that_expired_yesterday(): void
    {
        $this->freezeTime();
        $event = Event::factory()->create(['expire_at' => now()->subDay()->endOfDay()]);

        $this->assertFalse(Event::future()->get()->contains($event));
    }
}

I was also wondering if we should do a migration to make events.expire_at not a nullable field since events should probably always have an end date, even if we're forced to add some number of hours on import. I think we're currently adding 2 hours for Meetup or Eventbrite on import because one of them doesn't send the end date in the their API.

If we required expire_at I suppose we could make the migration update any existing records with null expire_at to be equal to active_at + 2 hours. It looked like only older events had a null value for expire_at.

I pushed another tweak to the test_robots_txt_disallows_all_in_non_production test.

I'm pretty sure the only reason I messed with it to begin with is I noticed it was failing on the GH action saying the 'local' environment didn't match its expectation of 'testing'.

The action does cp .env.ci .env. I'm guessing that's preempting what's set for the phpunit.xml APP_ENV ?

This is honestly the most testing I've messed with in a long time, if not the most I've ever messed with them, so I'm learning as I go.

Should we set phpunit.xml to force the environment to testing?
<env name="APP_ENV" value="testing" force="true"/>

Pinging @irby since he seemed to setup the .env.ci and many of the tests.