Skip to content
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

feat: move subscription to merged discussion #50

Closed
wants to merge 1 commit into from
Closed

feat: move subscription to merged discussion #50

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 55 additions & 3 deletions src/Api/Commands/MergeDiscussionHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,16 @@

use Flarum\Discussion\Discussion;
use Flarum\Discussion\DiscussionRepository;
use Flarum\Extension\ExtensionManager;
use Flarum\Foundation\ValidationException;
use Flarum\Post\Post;
use Flarum\User\UserRepository;
use FoF\MergeDiscussions\Events\DiscussionWasMerged;
use FoF\MergeDiscussions\Models\Redirection;
use FoF\MergeDiscussions\Validators\MergeDiscussionValidator;
use Illuminate\Database\ConnectionInterface;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Query\Builder;
use Illuminate\Events\Dispatcher;
use Illuminate\Support\Collection as SupportCollection;
use Throwable;
Expand All @@ -46,16 +49,30 @@ class MergeDiscussionHandler
*/
protected $validator;

/**
* @var ConnectionInterface
*/
protected $connection;

/**
* @var ExtensionManager
*/
protected $extensions;

public function __construct(
UserRepository $users,
DiscussionRepository $discussions,
Dispatcher $events,
MergeDiscussionValidator $validator
MergeDiscussionValidator $validator,
ConnectionInterface $connection,
ExtensionManager $extensions
) {
$this->users = $users;
$this->discussions = $discussions;
$this->events = $events;
$this->validator = $validator;
$this->connection = $connection;
$this->extensions = $extensions;
}

public function handle(MergeDiscussion $command)
Expand Down Expand Up @@ -93,7 +110,7 @@ public function handle(MergeDiscussion $command)
}

if ($command->merge) {
resolve('db.connection')->transaction(function () use ($discussions, $discussion, $command) {
$this->connection->transaction(function () use ($discussions, $discussion, $command) {
try {
// Set the relations using the bumped `number`, so we are sure we won't hit any integrity constraints
$discussion->push();
Expand Down Expand Up @@ -123,6 +140,41 @@ public function handle(MergeDiscussion $command)
$this->catchError($e, 'updating');
}

// Users who were following the merged discussions should now follow the new one.
if ($this->extensions->isEnabled('flarum-subscriptions')) {
// Move subscriptions to the new discussion.
$this->connection->table('discussion_user')
->whereIn('discussion_id', $discussions->pluck('id'))
->where('subscription', 'follow')
->whereNotExists(function (Builder $query) use ($discussion) {
$query->selectRaw(1)
->from('discussion_user', 'discussion_user2')
->where('discussion_user2.discussion_id', $discussion->id)
->whereColumn('discussion_user.user_id', 'discussion_user2.user_id');
})
->update([
'discussion_id' => $discussion->id,
'last_read_post_number' => $discussion->last_post_number,
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean the new merged discussion will be marked completely as read if any of the merged ones was being followed by the user? Even if the user hadn't read any of the posts involved in the merge?

Copy link
Contributor Author

@SychO9 SychO9 Jul 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yea, it only applies if they've never read the target discussion though, setting the number to 1 might be better in retrospect.

Copy link
Member

@dsevillamartin dsevillamartin Jul 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think i'd rather set it to the last post number where the user has read all previous posts - but that requires either checking old & new post numbers or created times. More complicated. And not limited to flarum/subscriptions either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that it's doable, post numbers change when merging from discussion A, B, C to discussion Y. Which post number to pick from A B C and how to pick it with its new value seems like a lot of complexity.

Also, since they haven't read discussion Y at all, setting a post number other than 1 means original posts they haven't read gets marked as read.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so what I described was keeping the original "last read post" references of each discussion and comparing their new post numbers, picking the smallest one as the one to save in the new discussion. This way it'd resume them at the point where new posts appear that they may have not read, with the ones they definitely had before them.

Eg.

Original Discussion ID Last Read Post's Old Number Last Read Post's New Number
1 4 6
2 1 3
3 1 8

We know they must've read posts #1-3 at least in the merged discussion, so we can set last post read number to 3. This doesn't necessarily mean the next post will not have been read, but it guarantees the ones beforehand were.

Of course, this could be also done by calculating which post is first encountered in the new numbering that wasn't read, but that seems more consuming.

Or we can do none of this cause It's not worth it 🤷. Not in the scope of this PR anyways.
I did start second guessing myself while I wrote this 😅 but I think what I wrote is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, thinking back again maybe it's too much to introduce this PR logic in the extension, might be better to keep it simple. How about just a MergingDiscussions event dispatched instead. That way it can be used for custom behavior without burdening current extension.

'subscription' => 'follow',
]);

// If the users have already read the new discussion, the query above will
// not have moved their subscriptions. We need to update them manually.
$this->connection->table('discussion_user')
->where('discussion_id', $discussion->id)
->whereNull('subscription')
->whereExists(function (Builder $query) use ($discussions) {
$query->selectRaw(1)
->from('discussion_user', 'discussion_user2')
->whereIn('discussion_user2.discussion_id', $discussions->pluck('id'))
->whereColumn('discussion_user.user_id', 'discussion_user2.user_id')
->where('subscription', 'follow');
})
->update([
'subscription' => 'follow',
]);
}

try {
foreach ($discussions as $d) {
Redirection::build($d, $discussion);
Expand Down Expand Up @@ -171,7 +223,7 @@ private function fixPostsNumber(Discussion $discussion): void

$discussion->setRelation('posts', $discussion->posts->sortBy('number'));

resolve('db.connection')->transaction(function () use ($discussion) {
$this->connection->transaction(function () use ($discussion) {
try {
$discussion->push();
} catch (Throwable $e) {
Expand Down