diff --git a/app/Queries/MentionedOrganizationsQuery.php b/app/Queries/MentionedOrganizationsQuery.php index a147191e0c..4fcb38248e 100644 --- a/app/Queries/MentionedOrganizationsQuery.php +++ b/app/Queries/MentionedOrganizationsQuery.php @@ -6,7 +6,6 @@ use App\Models\Organization; use App\Services\Mentions; -use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; @@ -19,7 +18,8 @@ public function __construct(private Mentions $mentions) /** * The organizations mentioned in the text, by lowercase handle. A handle that is also a user's mentions the user, - * so it is left out. + * so it is left out. A stale profile may still hold the same handle in another case, so the case it is written in + * wins, then always the same one, like organization pages. * * @param array $mentionedUsers * @return array @@ -32,9 +32,16 @@ public function get(string $body, array $mentionedUsers): array return []; } + $written = $this->mentions->handlesAsWritten($body); + return Organization::query() - ->whereHas('profile', fn (Builder $query): Builder => $query->whereIn(DB::raw('LOWER(handle)'), $handles)) + ->select('organizations.*') + ->join('profiles', 'profiles.id', '=', 'organizations.id') + ->whereIn(DB::raw('LOWER(profiles.handle)'), $handles) + ->orderBy('profiles.handle') ->get() + ->sortByDesc(fn (Organization $organization): bool => in_array($organization->handle, $written, true)) + ->uniqueStrict(fn (Organization $organization): string => Str::lower($organization->handle)) ->mapWithKeys(fn (Organization $organization): array => [Str::lower($organization->handle) => $organization->id]) ->all(); } diff --git a/app/Services/Mentions.php b/app/Services/Mentions.php index 07dbba4e82..773a79b21d 100644 --- a/app/Services/Mentions.php +++ b/app/Services/Mentions.php @@ -23,6 +23,17 @@ public function __construct(private CodeSpans $codeSpans) * @return list */ public function handles(string $text): array + { + return array_values(array_unique(array_map(strtolower(...), $this->handlesAsWritten($text)))); + } + + /** + * Finds the handles mentioned as "@handle" in the text by the same rules, in the case they are written, once each, in + * order. + * + * @return list + */ + public function handlesAsWritten(string $text): array { if (! str_contains($text, '@')) { return []; @@ -30,7 +41,7 @@ public function handles(string $text): array preg_match_all(self::Pattern, $this->codeSpans->withoutCode($text), $matches); - return array_values(array_unique(array_map(strtolower(...), $matches[1]))); + return array_values(array_unique($matches[1])); } /** diff --git a/tests/Feature/OrganizationMentionsTest.php b/tests/Feature/OrganizationMentionsTest.php index f9c786ef46..f58a19ddbf 100644 --- a/tests/Feature/OrganizationMentionsTest.php +++ b/tests/Feature/OrganizationMentionsTest.php @@ -6,8 +6,10 @@ use App\Jobs\LinkCommitTopics; use App\Models\Commit; use App\Models\Organization; +use App\Models\Profile; use App\Models\User; use Illuminate\Support\Facades\Queue; +use Illuminate\Support\Str; it('links known organizations right away, a user of the same handle first', function (): void { Queue::fake(); @@ -28,6 +30,58 @@ ->assertInertia(fn ($page) => $page->where('commit.organization_mentions.laravel', $laravel->id)); }); +it('links the organization in the case it is written when a stale one holds the handle in another case', function (string $written, string $linked): void { + config(['app.admins' => ['admin']]); + Queue::fake(); + + $author = User::factory()->create(['handle' => 'admin']); + $organizations = [ + 'laravel' => Organization::factory()->create(['handle' => 'laravel']), + 'Laravel' => Organization::factory()->create(['handle' => 'Laravel']), + ]; + + $this->actingAs($author)->post('/commits', ['body' => "Thanks @{$written}"])->assertRedirect(); + + $commit = $author->commits()->sole(); + + expect($commit->organization_mentions)->toBe(['laravel' => $organizations[$linked]->id]); + + $this->getJson("/api/v1/organizations/{$written}/mentions")->assertOk()->assertJsonPath('data.*.id', [$commit->id]); +})->with([ + 'lowercase' => ['laravel', 'laravel'], + 'capitalized' => ['Laravel', 'Laravel'], +]); + +it('links always the same organization when no stale one holds the handle in the case it is written', function (): void { + Queue::fake(); + + $author = User::factory()->create(); + Organization::factory()->create(['handle' => 'laravel']); + Organization::factory()->create(['handle' => 'Laravel']); + + $this->actingAs($author)->post('/commits', ['body' => 'Thanks @LARAVEL'])->assertRedirect(); + $this->actingAs($author)->post('/commits', ['body' => 'Thanks again @LARAVEL'])->assertRedirect(); + + $location = (string) $this->get('/@LARAVEL')->assertMovedPermanently()->headers->get('Location'); + $page = Profile::query()->where('handle', Str::after($location, '/@'))->sole(); + + foreach ($author->commits()->get() as $commit) { + expect($commit->organization_mentions)->toBe(['laravel' => $page->id]); + } +}); + +it('links organizations whose handles only look like the same number', function (): void { + Queue::fake(); + + $author = User::factory()->create(); + $hundred = Organization::factory()->create(['handle' => '100']); + $scientific = Organization::factory()->create(['handle' => '1e2']); + + $this->actingAs($author)->post('/commits', ['body' => 'Thanks @100 and @1e2'])->assertRedirect(); + + expect($author->commits()->sole()->organization_mentions)->toBe(['100' => $hundred->id, '1e2' => $scientific->id]); +}); + it('asks GitHub about the other handles of a commit approved right away', function (): void { config(['app.admins' => ['admin']]); Queue::fake(); diff --git a/tests/Unit/Services/MentionsTest.php b/tests/Unit/Services/MentionsTest.php index 9ef2e274b7..fdfef4e619 100644 --- a/tests/Unit/Services/MentionsTest.php +++ b/tests/Unit/Services/MentionsTest.php @@ -11,6 +11,14 @@ ->toBe(['nunomaduro', 'taylorotwell']); }); +it('finds mentions in the case they are written, once each, in order', function (): void { + $mentions = resolve(Mentions::class); + + expect($mentions->handlesAsWritten('@NunoMaduro and @taylorotwell, thanks! cc @NunoMaduro and @nunomaduro, not `@Override`')) + ->toBe(['NunoMaduro', 'taylorotwell', 'nunomaduro']) + ->and($mentions->handlesAsWritten('No mentions here'))->toBe([]); +}); + it('finds mentions next to punctuation', function (string $text, array $handles): void { $mentions = resolve(Mentions::class);