From b30022da4b869ccad8412678941a8551f7601a2e Mon Sep 17 00:00:00 2001 From: Marien Fressinaud Date: Thu, 17 Oct 2024 17:27:59 +0200 Subject: [PATCH 1/2] dev: Remove the NewsPicker service With the recent changes, this class became useless. --- src/controllers/News.php | 7 +- src/services/NewsPicker.php | 32 ---------- .../dao/links/NewsQueriesTest.php} | 64 ++++++++----------- 3 files changed, 28 insertions(+), 75 deletions(-) delete mode 100644 src/services/NewsPicker.php rename tests/{services/NewsPickerTest.php => models/dao/links/NewsQueriesTest.php} (87%) diff --git a/src/controllers/News.php b/src/controllers/News.php index 606d500c..719f2053 100644 --- a/src/controllers/News.php +++ b/src/controllers/News.php @@ -6,7 +6,6 @@ use Minz\Response; use App\auth; use App\models; -use App\services; use App\utils; /** @@ -78,8 +77,7 @@ public function create(Request $request): Response ]); } - $news_picker = new services\NewsPicker($user); - $links = $news_picker->pick(max: 50); + $links = models\Link::listFromFollowedCollections($user->id, max:50); $news = $user->news(); @@ -123,8 +121,7 @@ public function showAvailable(Request $request): Response ]); } - $news_picker = new services\NewsPicker($user); - $links = $news_picker->pick(max: 2); + $links = models\Link::listFromFollowedCollections($user->id, max:2); return Response::json(200, [ 'available' => count($links) > 0, diff --git a/src/services/NewsPicker.php b/src/services/NewsPicker.php deleted file mode 100644 index 1dd7daf3..00000000 --- a/src/services/NewsPicker.php +++ /dev/null @@ -1,32 +0,0 @@ - - * @license http://www.gnu.org/licenses/agpl-3.0.en.html AGPL - */ -class NewsPicker -{ - private models\User $user; - - public function __construct(models\User $user) - { - $this->user = $user; - } - - /** - * Pick and return a set of links relevant for news. - * - * @return models\Link[] - */ - public function pick(int $max = 25): array - { - return models\Link::listFromFollowedCollections($this->user->id, $max); - } -} diff --git a/tests/services/NewsPickerTest.php b/tests/models/dao/links/NewsQueriesTest.php similarity index 87% rename from tests/services/NewsPickerTest.php rename to tests/models/dao/links/NewsQueriesTest.php index eb9dc896..1da31752 100644 --- a/tests/services/NewsPickerTest.php +++ b/tests/models/dao/links/NewsQueriesTest.php @@ -1,6 +1,6 @@ other_user = UserFactory::create(); } - public function testPickSelectsFromFollowed(): void + public function testListFromFollowedCollectionsSelectsFromFollowed(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); $this->freeze($now); $published_at1 = \Minz\Time::ago(3, 'days'); $published_at2 = \Minz\Time::ago(1, 'days'); - $news_picker = new NewsPicker($this->user); $link1 = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => false, @@ -62,7 +61,7 @@ public function testPickSelectsFromFollowed(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(2, count($links)); $this->assertSame($link2->id, $links[0]->id); @@ -73,7 +72,7 @@ public function testPickSelectsFromFollowed(): void $this->assertSame($collection->id, $links[1]->source_news_resource_id); } - public function testPickSelectsHiddenLinkIfCollectionIsShared(): void + public function testListFromFollowedCollectionsSelectsHiddenLinkIfCollectionIsShared(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -81,7 +80,6 @@ public function testPickSelectsHiddenLinkIfCollectionIsShared(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => true, @@ -105,7 +103,7 @@ public function testPickSelectsHiddenLinkIfCollectionIsShared(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(1, count($links)); $this->assertSame($link->id, $links[0]->id); @@ -113,7 +111,7 @@ public function testPickSelectsHiddenLinkIfCollectionIsShared(): void $this->assertSame($collection->id, $links[0]->source_news_resource_id); } - public function testPickSelectsFromPrivateCollectionIfShared(): void + public function testListFromFollowedCollectionsSelectsFromPrivateCollectionIfShared(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -121,7 +119,6 @@ public function testPickSelectsFromPrivateCollectionIfShared(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => false, @@ -145,7 +142,7 @@ public function testPickSelectsFromPrivateCollectionIfShared(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(1, count($links)); $this->assertSame($link->id, $links[0]->id); @@ -153,7 +150,7 @@ public function testPickSelectsFromPrivateCollectionIfShared(): void $this->assertSame($collection->id, $links[0]->source_news_resource_id); } - public function testPickRespectsFromFollowedIfOldLinksButWithTimeFilterAll(): void + public function testListFromFollowedCollectionsRespectsFromFollowedIfOldLinksButWithTimeFilterAll(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -165,7 +162,6 @@ public function testPickRespectsFromFollowedIfOldLinksButWithTimeFilterAll(): vo // when the user started to follow the collection $delta_followed_days = $this->fake('numberBetween', 0, 7); $followed_at = \Minz\Time::ago($days_ago - $delta_followed_days, 'days'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => false, @@ -187,7 +183,7 @@ public function testPickRespectsFromFollowedIfOldLinksButWithTimeFilterAll(): vo 'time_filter' => 'all', ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(1, count($links)); $this->assertSame($link->id, $links[0]->id); @@ -195,7 +191,7 @@ public function testPickRespectsFromFollowedIfOldLinksButWithTimeFilterAll(): vo $this->assertSame($collection->id, $links[0]->source_news_resource_id); } - public function testPickDoesNotPickFromFollowedIfTooOld(): void + public function testListFromFollowedCollectionsDoesNotPickFromFollowedIfTooOld(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -203,7 +199,6 @@ public function testPickDoesNotPickFromFollowedIfTooOld(): void /** @var int */ $days_ago = $this->fake('numberBetween', 8, 180); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => false, @@ -223,12 +218,12 @@ public function testPickDoesNotPickFromFollowedIfTooOld(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } - public function testPickDoesNotSelectFromFollowedIfTooOldWithTimeFilterStrict(): void + public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfTooOldWithTimeFilterStrict(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -236,7 +231,6 @@ public function testPickDoesNotSelectFromFollowedIfTooOldWithTimeFilterStrict(): /** @var int */ $hours_ago = $this->fake('numberBetween', 25, 72); $published_at = \Minz\Time::ago($hours_ago, 'hours'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => false, @@ -257,12 +251,12 @@ public function testPickDoesNotSelectFromFollowedIfTooOldWithTimeFilterStrict(): 'time_filter' => 'strict', ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } - public function testPickDoesNotSelectFromFollowedIfLinkIsHidden(): void + public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfLinkIsHidden(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -270,7 +264,6 @@ public function testPickDoesNotSelectFromFollowedIfLinkIsHidden(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => true, @@ -290,12 +283,12 @@ public function testPickDoesNotSelectFromFollowedIfLinkIsHidden(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } - public function testPickDoesNotSelectFromFollowedIfCollectionIsPrivate(): void + public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfCollectionIsPrivate(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -303,7 +296,6 @@ public function testPickDoesNotSelectFromFollowedIfCollectionIsPrivate(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->other_user->id, 'is_hidden' => false, @@ -323,12 +315,12 @@ public function testPickDoesNotSelectFromFollowedIfCollectionIsPrivate(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } - public function testPickDoesNotSelectFromFollowedIfUrlInBookmarks(): void + public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfUrlInBookmarks(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -336,7 +328,6 @@ public function testPickDoesNotSelectFromFollowedIfUrlInBookmarks(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $bookmarks = $this->user->bookmarks(); /** @var string */ $url = $this->fake('url'); @@ -368,12 +359,12 @@ public function testPickDoesNotSelectFromFollowedIfUrlInBookmarks(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } - public function testPickDoesNotSelectFromFollowedIfUrlInReadList(): void + public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfUrlInReadList(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -381,7 +372,6 @@ public function testPickDoesNotSelectFromFollowedIfUrlInReadList(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $read_list = $this->user->readList(); /** @var string */ $url = $this->fake('url'); @@ -413,12 +403,12 @@ public function testPickDoesNotSelectFromFollowedIfUrlInReadList(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } - public function testPickDoesNotSelectFromFollowedIfUrlInNeverList(): void + public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfUrlInNeverList(): void { /** @var \DateTimeImmutable */ $now = $this->fake('dateTime'); @@ -426,7 +416,6 @@ public function testPickDoesNotSelectFromFollowedIfUrlInNeverList(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $never_list = $this->user->neverList(); /** @var string */ $url = $this->fake('url'); @@ -458,12 +447,12 @@ public function testPickDoesNotSelectFromFollowedIfUrlInNeverList(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } - public function testPickDoesNotSelectFromFollowedIfLinkIsOwned(): void + public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfLinkIsOwned(): void { // This is a very particular use case where the user got write access // to a collection while he was following it (or followed it @@ -476,7 +465,6 @@ public function testPickDoesNotSelectFromFollowedIfLinkIsOwned(): void /** @var int */ $days_ago = $this->fake('numberBetween', 0, 7); $published_at = \Minz\Time::ago($days_ago, 'days'); - $news_picker = new NewsPicker($this->user); $link = LinkFactory::create([ 'user_id' => $this->user->id, 'is_hidden' => false, @@ -496,7 +484,7 @@ public function testPickDoesNotSelectFromFollowedIfLinkIsOwned(): void 'collection_id' => $collection->id, ]); - $links = $news_picker->pick(); + $links = models\Link::listFromFollowedCollections($this->user->id, max: 50); $this->assertSame(0, count($links)); } From 41014681fa447205b1932cbd533d7b2a8965942f Mon Sep 17 00:00:00 2001 From: Marien Fressinaud Date: Thu, 17 Oct 2024 17:33:32 +0200 Subject: [PATCH 2/2] imp: Improve performance when checking if news are available --- locales/fr_FR/LC_MESSAGES/main.mo | Bin 58331 -> 58331 bytes locales/fr_FR/LC_MESSAGES/main.po | 6 +- src/controllers/News.php | 4 +- src/models/dao/links/NewsQueries.php | 72 +++++++++++++++++++++ tests/models/dao/links/NewsQueriesTest.php | 54 ++++++++++++++++ 5 files changed, 130 insertions(+), 6 deletions(-) diff --git a/locales/fr_FR/LC_MESSAGES/main.mo b/locales/fr_FR/LC_MESSAGES/main.mo index 5c3b48c7f78f68e994a8e7090fbf3980a3e42b00..6b60c42962b8e34dad1db2cb2fe713b0c75c9b99 100644 GIT binary patch delta 23 fcmcb8ocZ=~<_)_VI1J4d49%^K%{CuukW2;ud(#O) delta 23 fcmcb8ocZ=~<_)_VI1Ee_3@xn;EjAx&kW2;ud$I{X diff --git a/locales/fr_FR/LC_MESSAGES/main.po b/locales/fr_FR/LC_MESSAGES/main.po index 064c3efb..18432b51 100644 --- a/locales/fr_FR/LC_MESSAGES/main.po +++ b/locales/fr_FR/LC_MESSAGES/main.po @@ -1,8 +1,8 @@ msgid "" msgstr "" "Project-Id-Version: Flus\n" -"POT-Creation-Date: 2024-10-04 19:18+0200\n" -"PO-Revision-Date: 2024-10-04 19:18+0200\n" +"POT-Creation-Date: 2024-10-17 17:36+0200\n" +"PO-Revision-Date: 2024-10-17 17:36+0200\n" "Last-Translator: Marien Fressinaud \n" "Language-Team: \n" "Language: fr_FR\n" @@ -50,7 +50,7 @@ msgstr "Afficher" #: controllers/Feeds.php:107 controllers/Groups.php:90 #: controllers/Links.php:293 controllers/Links.php:465 #: controllers/Mastodon.php:211 controllers/Mastodon.php:297 -#: controllers/News.php:77 controllers/Passwords.php:89 +#: controllers/News.php:76 controllers/Passwords.php:89 #: controllers/Passwords.php:192 controllers/Registrations.php:103 #: controllers/Sessions.php:85 controllers/Support.php:70 #: controllers/collections/Filters.php:105 diff --git a/src/controllers/News.php b/src/controllers/News.php index 719f2053..0502a6f2 100644 --- a/src/controllers/News.php +++ b/src/controllers/News.php @@ -121,10 +121,8 @@ public function showAvailable(Request $request): Response ]); } - $links = models\Link::listFromFollowedCollections($user->id, max:2); - return Response::json(200, [ - 'available' => count($links) > 0, + 'available' => models\Link::anyFromFollowedCollections($user->id), ]); } } diff --git a/src/models/dao/links/NewsQueries.php b/src/models/dao/links/NewsQueries.php index 67ea113f..909760b3 100644 --- a/src/models/dao/links/NewsQueries.php +++ b/src/models/dao/links/NewsQueries.php @@ -96,6 +96,78 @@ public static function listFromFollowedCollections(string $user_id, int $max): a return self::fromDatabaseRows($results); } + /** + * Return whether there are any public links listed in followed collections + * of the given user. + */ + public static function anyFromFollowedCollections(string $user_id): bool + { + $values = [ + ':user_id' => $user_id, + ':until_hard_limit' => \Minz\Time::ago(1, 'year')->format(Database\Column::DATETIME_FORMAT), + ':until_strict' => \Minz\Time::ago(1, 'day')->format(Database\Column::DATETIME_FORMAT), + ':until_normal' => \Minz\Time::ago(1, 'week')->format(Database\Column::DATETIME_FORMAT), + ]; + + $sql = <<= :until_hard_limit + AND ( + (fc.time_filter = 'strict' AND lc.created_at >= :until_strict) OR + (fc.time_filter = 'normal' AND lc.created_at >= :until_normal) OR + (fc.time_filter = 'all' AND lc.created_at >= fc.created_at - INTERVAL '1 week') + ) + ) + SQL; + + $database = Database::get(); + $statement = $database->prepare($sql); + $statement->execute($values); + + return $statement->fetch() !== false; + } + /** * Mark the relevant links to be grouped by sources in the given collection. * diff --git a/tests/models/dao/links/NewsQueriesTest.php b/tests/models/dao/links/NewsQueriesTest.php index 1da31752..1d3eac3d 100644 --- a/tests/models/dao/links/NewsQueriesTest.php +++ b/tests/models/dao/links/NewsQueriesTest.php @@ -488,4 +488,58 @@ public function testListFromFollowedCollectionsDoesNotSelectFromFollowedIfLinkIs $this->assertSame(0, count($links)); } + + public function testAnyFromFollowedCollectionsCanReturnTrue(): void + { + $published_at = \Minz\Time::ago(1, 'day'); + $link = LinkFactory::create([ + 'user_id' => $this->other_user->id, + 'is_hidden' => false, + ]); + $collection = CollectionFactory::create([ + 'user_id' => $this->other_user->id, + 'type' => 'collection', + 'is_public' => true, + ]); + LinkToCollectionFactory::create([ + 'created_at' => $published_at, + 'collection_id' => $collection->id, + 'link_id' => $link->id, + ]); + FollowedCollectionFactory::create([ + 'user_id' => $this->user->id, + 'collection_id' => $collection->id, + ]); + + $result = models\Link::anyFromFollowedCollections($this->user->id); + + $this->assertTrue($result); + } + + public function testAnyFromFollowedCollectionsCanReturnFalse(): void + { + $published_at = \Minz\Time::ago(1, 'day'); + $link = LinkFactory::create([ + 'user_id' => $this->other_user->id, + 'is_hidden' => true, // Note the link is hidden + ]); + $collection = CollectionFactory::create([ + 'user_id' => $this->other_user->id, + 'type' => 'collection', + 'is_public' => true, + ]); + LinkToCollectionFactory::create([ + 'created_at' => $published_at, + 'collection_id' => $collection->id, + 'link_id' => $link->id, + ]); + FollowedCollectionFactory::create([ + 'user_id' => $this->user->id, + 'collection_id' => $collection->id, + ]); + + $result = models\Link::anyFromFollowedCollections($this->user->id); + + $this->assertFalse($result); + } }