Skip to content

Handle Carbon 3 timestamp parsing and add boundary tests #1132

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.

Sign up for GitHub

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

Open
dati18 wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Handle Carbon 3 timestamp parsing and add boundary tests #1132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
069c041
Handle Carbon 3 timestamp parsing and add coverage for timestamp hand…
dati18 May 19, 2026
a51fd9c
Add test case to cover wiki noti threshold boundary
dati18 May 20, 2026
d86dba6
Add test second-precision lastEdit threshold in platform stats summary
dati18 May 20, 2026
842a701
Add test for fractional day diffs in conversion metrics
dati18 May 20, 2026
e81d3e8
fix linting error
dati18 May 20, 2026
5d81db8
remove unwanted changes
dati18 May 20, 2026
da79c7e
Remove accidental bootstrap/providers.php from branch
dati18 May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions app/Helper/MWTimestampHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,13 @@ class MWTimestampHelper {
private const MWTimestampFormat = 'YmdHis';

public static function getCarbonFromMWTimestamp(string $MWTimestamp): CarbonImmutable {
$carbon = CarbonImmutable::createFromFormat(self::MWTimestampFormat, $MWTimestamp);
if ($carbon === null) {
try {
Copy link
Copy Markdown
Contributor

@deer-wmde deer-wmde May 22, 2026

Choose a reason for hiding this comment

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

I'm a bit unclear about this part.

  1. since Carbon 3 will throw an exception when createFormFormat, I'm not certain it helps to catch it just so we can throw our own. This could even make it harder to spot the reason for why it failed.
  2. the second check also seems obsolete since we either get a CarbonImmutable or an Exception gets thrown

So I think this method could be as simple as just line 19 - if something goes wrong an exception gets thrown as it used to. Is there more context for this additional logic that I'm not aware of?

Copy link
Copy Markdown
Contributor Author

@dati18 dati18 May 22, 2026

Choose a reason for hiding this comment

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

you're right to question it, it's an overthinking part from me. My logic is createFromFormat() may throw a non-instance (null/false), so an extra instanceof guard prevents type error when returning $carbon.

$carbon = CarbonImmutable::createFromFormat(self::MWTimestampFormat, $MWTimestamp);
} catch (InvalidFormatException $exception) {
throw new InvalidFormatException('Unable to create Carbon object', 0, $exception);
}

if (!$carbon instanceof CarbonImmutable) {
throw new InvalidFormatException('Unable to create Carbon object');
}

Expand Down
1 change: 1 addition & 0 deletions tests/Helper/MWTimestampHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public function testGetCarbonFromMWTimestamp() {

public function testGetCarbonFromMWTimestampWithInvalidTimestamp() {
$this->expectException(InvalidFormatException::class);
$this->expectExceptionMessage('Unable to create Carbon object');
Copy link
Copy Markdown
Contributor

@deer-wmde deer-wmde May 22, 2026

Choose a reason for hiding this comment

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

As line 21 is expecting the Exception already, I struggle to see how this check helps us

Copy link
Copy Markdown
Contributor Author

@dati18 dati18 May 22, 2026

Choose a reason for hiding this comment

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

I just want to lock down the exact error message. a bit redundant, yes... WHat do you think? keep it or remove it?


$invalidMwTimestamp = 'invalid_timestamp';
MWTimestampHelper::getCarbonFromMWTimestamp($invalidMwTimestamp);
Expand Down
46 changes: 46 additions & 0 deletions tests/Jobs/PlatformStatsSummaryJobTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,4 +274,50 @@ public function testCreationStats() {
);

}

public function testPrepareStatsTreatsSecondPrecisionTimestampAtThresholdAsActive() {
$currentTime = CarbonImmutable::now();

$wiki = Wiki::factory()->create(['deleted_at' => null, 'domain' => 'thresholdtest.com']);
WikiDb::create([
'name' => 'mwdb_threshold_' . $wiki->id,
'user' => 'user',
'password' => 'password',
'version' => 'version',
'prefix' => 'prefix',
'wiki_id' => $wiki->id,
]);

Http::fake([
$this->mwBackendHost . '/w/api.php?action=query&list=allpages&apnamespace=122&apcontinue=&aplimit=max&format=json' => Http::response([
'query' => ['allpages' => []],
], 200),
$this->mwBackendHost . '/w/api.php?action=query&list=allpages&apnamespace=120&apcontinue=&aplimit=max&format=json' => Http::response([
'query' => ['allpages' => []],
], 200),
]);

$job = new PlatformStatsSummaryJob;
(function ($resolver): void {
$this->mwHostResolver = $resolver;
})->call($job, $this->mockMwHostResolver);

$groups = $job->prepareStats([
[
'wiki' => 'thresholdtest.com',
'edits' => 1,
'pages' => 1,
'users' => 1,
'active_users' => 1,
'lastEdit' => MWTimestampHelper::getMWTimestampFromCarbon(
$currentTime->subSeconds(config('wbstack.platform_summary_inactive_threshold'))
),
'first100UsingOauth' => '0',
'platform_summary_version' => 'v1',
],
], [$wiki]);

$this->assertSame(1, $groups['edited_last_90_days']);
$this->assertSame(0, $groups['not_edited_last_90_days']);
}
}
20 changes: 19 additions & 1 deletion tests/Jobs/SendEmptyWikiNotificationsJobTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function testEmptyWikiNotificationsSendNotification() {
Notification::fake();
$user = User::factory()->create(['verified' => true]);
$wiki = Wiki::factory()->create(['created_at' => $thresholdDaysAgo]);
$manager = WikiManager::factory()->create(['wiki_id' => $wiki->id, 'user_id' => $user->id]);
WikiManager::factory()->create(['wiki_id' => $wiki->id, 'user_id' => $user->id]);
$wiki->wikiLifecycleEvents()->updateOrCreate(['first_edited' => null]);

$job = new SendEmptyWikiNotificationsJob;
Expand All @@ -50,6 +50,24 @@ public function testEmptyWikiNotificationsSendNotification() {
);
}

// empty wikis, that are almost old enough (29 days and 23 hrs)
public function testEmptyWikiNotificationsNotSendNotification() {
$thresholdDaysAgo = Carbon::now()
->subDays((config('wbstack.wiki_empty_notification_threshold') - 1))
->subHours(23)
->toDateTimeString();

Notification::fake();
$user = User::factory()->create(['verified' => true]);
$wiki = Wiki::factory()->create(['created_at' => $thresholdDaysAgo]);
WikiManager::factory()->create(['wiki_id' => $wiki->id, 'user_id' => $user->id]);
$wiki->wikiLifecycleEvents()->updateOrCreate(['first_edited' => null]);

$job = new SendEmptyWikiNotificationsJob;
$this->assertFalse($job->checkIfWikiIsOldAndEmpty($wiki));
$job->handle();
}

// fresh wiki that does not have lifecycle event records yet
public function testEmptyWikiNotificationsFreshWiki() {
$now = Carbon::now()->toDateTimeString();
Expand Down
37 changes: 35 additions & 2 deletions tests/Routes/Wiki/ConversionMetricTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ private function createTestWiki($name, $createdWeeksAgo, $firstEditedWeeksAgo, $
$wiki->created_at = $current_date->subWeeks($createdWeeksAgo);
$events = $wiki->wikiLifecycleEvents();
$update = [];
if ($lastEditedWeeksAgo) {
if ($lastEditedWeeksAgo !== null) {
$update['last_edited'] = $current_date->subWeeks($lastEditedWeeksAgo);
}
if ($firstEditedWeeksAgo) {
if ($firstEditedWeeksAgo !== null) {
$update['first_edited'] = $current_date->subWeeks($firstEditedWeeksAgo);
}
$events->updateOrCreate($update);
Expand Down Expand Up @@ -134,6 +134,39 @@ public function testDownloadJson() {
);
}

public function testDownloadJsonTruncatesFractionalDayDiffs() {
$currentDate = CarbonImmutable::now();
$createdAt = $currentDate->subDays(200)->subHours(12); // 200.5 days ago
$firstEditedAt = $createdAt->addDays(1)->addHours(12); // 1.5 days after
$lastEditedAt = $currentDate->subDays(100); // 100 days ago

$wiki = Wiki::factory()->create([
'domain' => 'fractional.days.cloud',
'sitename' => 'Fractional Days Site',
]);
WikiSiteStats::factory()->create([
'wiki_id' => $wiki->id,
'pages' => 77,
'activeusers' => 2,
]);
$wiki->created_at = $createdAt;
$wiki->wikiLifecycleEvents()->updateOrCreate([
'first_edited' => $firstEditedAt,
'last_edited' => $lastEditedAt,
]);
$wiki->save();

$response = $this->getJson($this->route);

$response->assertStatus(200);
$response->assertJsonFragment([
'domain' => 'fractional.days.cloud',
'time_to_engage_days' => 1,
'time_before_wiki_abandoned_days' => 100,
'number_of_active_editors' => 2,
]);
}

public function testFunctionalWithMissingLifecycleEventsandStats() {
$wiki = Wiki::factory()->create([
'domain' => 'very.new.wikibase.cloud', 'sitename' => 'bsite',
Expand Down
Loading