From d44b1013731440988f29af6282fd76a4308de392 Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Wed, 1 Jul 2015 16:31:06 +0930 Subject: [PATCH] Clean up merging stuff --- src copy/Core/Posts/EventPost.php | 27 +++ .../Core/Support}/MergeableInterface.php | 0 .../Core/Support}/MergeableTrait.php | 0 src/Core/Models/Discussion.php | 168 ++++++++---------- src/Core/Models/DiscussionRenamedPost.php | 49 ++--- src/Core/Models/EventPost.php | 12 +- src/Core/Models/Mergeable.php | 24 +++ 7 files changed, 162 insertions(+), 118 deletions(-) create mode 100755 src copy/Core/Posts/EventPost.php rename {src/Core/Models => src copy/Core/Support}/MergeableInterface.php (100%) rename {src/Core/Models => src copy/Core/Support}/MergeableTrait.php (100%) create mode 100644 src/Core/Models/Mergeable.php diff --git a/src copy/Core/Posts/EventPost.php b/src copy/Core/Posts/EventPost.php new file mode 100755 index 000000000..6c392b29c --- /dev/null +++ b/src copy/Core/Posts/EventPost.php @@ -0,0 +1,27 @@ +attributes['content'] = json_encode($value); + } +} diff --git a/src/Core/Models/MergeableInterface.php b/src copy/Core/Support/MergeableInterface.php similarity index 100% rename from src/Core/Models/MergeableInterface.php rename to src copy/Core/Support/MergeableInterface.php diff --git a/src/Core/Models/MergeableTrait.php b/src copy/Core/Support/MergeableTrait.php similarity index 100% rename from src/Core/Models/MergeableTrait.php rename to src copy/Core/Support/MergeableTrait.php diff --git a/src/Core/Models/Discussion.php b/src/Core/Models/Discussion.php index 7444084bc..385a82edf 100755 --- a/src/Core/Models/Discussion.php +++ b/src/Core/Models/Discussion.php @@ -33,8 +33,6 @@ class Discussion extends Model 'last_post_number' => 'integer' ]; - protected static $relationships = []; - /** * The table associated with the model. * @@ -47,20 +45,6 @@ class Discussion extends Model * * @var array */ - - /** - * An array of posts that have been added during this request. - * - * @var \Flarum\Core\Models\Post[] - */ - public $addedPosts = []; - - /** - * An array of posts that have been removed during this request. - * - * @var \Flarum\Core\Models\Post[] - */ - public $removedPosts = []; protected static $dates = ['start_time', 'last_time']; /** @@ -71,7 +55,14 @@ class Discussion extends Model protected static $stateUser; /** - * Raise an event when a discussion is deleted. + * An array of callables that apply constraints to the visiblePosts query. + * + * @var callable[] + */ + protected static $visiblePostsScopes = []; + + /** + * Boot the model. * * @return void */ @@ -82,28 +73,31 @@ class Discussion extends Model static::deleted(function ($discussion) { $discussion->raise(new DiscussionWasDeleted($discussion)); + // Delete all of the posts in the discussion. Before we delete them + // in a big batch query, we will loop through them and raise a + // PostWasDeleted event for each post. $posts = $discussion->posts()->allTypes(); foreach ($posts->get() as $post) { - $post->setRelation('discussion', $discussion); - $discussion->raise(new PostWasDeleted($post)); } $posts->delete(); + // Delete all of the 'state' records for all of the users who have + // read the discussion. $discussion->readers()->detach(); }); } /** - * Create a new instance. + * Start a new discussion. Raises the DiscussionWasStarted event. * - * @param string $title - * @param \Flarum\Core\Models\User $user - * @return static + * @param string $title + * @param \Flarum\Core\Models\User $user + * @return \Flarum\Core\Models\Discussion */ - public static function start($title, $user) + public static function start($title, User $user) { $discussion = new static; @@ -117,13 +111,13 @@ class Discussion extends Model } /** - * Rename the discussion. + * Rename the discussion. Raises the DiscussionWasRenamed event. * - * @param string $title - * @param \Flarum\Core\Models\User $user + * @param string $title + * @param \Flarum\Core\Models\User $user * @return $this */ - public function rename($title, $user) + public function rename($title, User $user) { if ($this->title !== $title) { $oldTitle = $this->title; @@ -138,7 +132,7 @@ class Discussion extends Model /** * Set the discussion's start post details. * - * @param \Flarum\Core\Models\Post $post + * @param \Flarum\Core\Models\Post $post * @return $this */ public function setStartPost(Post $post) @@ -153,7 +147,7 @@ class Discussion extends Model /** * Set the discussion's last post details. * - * @param \Flarum\Core\Models\Post $post + * @param \Flarum\Core\Models\Post $post * @return $this */ public function setLastPost(Post $post) @@ -173,7 +167,7 @@ class Discussion extends Model */ public function refreshLastPost() { - if ($lastPost = $this->comments()->orderBy('time', 'desc')->first()) { + if ($lastPost = $this->comments()->latest('time')->first()) { $this->setLastPost($lastPost); } @@ -205,45 +199,23 @@ class Discussion extends Model } /** - * Specify that a post was added to this discussion during this request - * for later retrieval. + * Save a post, attempting to merge it with the discussion's last post. * - * @param \Flarum\Core\Models\Post $post - * @return void - */ - public function postWasAdded(Post $post) - { - $this->addedPosts[] = $post; - } - - /** - * Specify that a post was removed from this discussion during this - * request for later retrieval. + * The merge logic is delegated to the new post. (As an example, a + * DiscussionRenamedPost will merge if adjacent to another + * DiscussionRenamedPost, and delete if the title has been reverted + * completely.) * - * @param \Flarum\Core\Models\Post $post - * @return void + * @param \Flarum\Core\Posts\Post $post The post to save. + * @return \Flarum\Core\Posts\Post The resulting post. It may or may not be + * the same post as was originally intended to be saved. It also may not + * exist, if the merge logic resulted in deletion. */ - public function postWasRemoved(Post $post) + public function mergePost(Mergable $post) { - $this->removedPosts[] = $post->id; - } + $lastPost = $this->posts()->latest('time')->first(); - public function addPost(Post $post) - { - if ($post instanceof MergeableInterface) { - $lastPost = $this->posts()->orderBy('time', 'desc')->first(); - $post = $post->saveAfter($lastPost); - } else { - $post->save(); - } - - if ($post->exists) { - $this->postWasAdded($post); - } else { - $this->postWasRemoved($post); - } - - return $post; + return $post->saveAfter($lastPost); } /** @@ -256,13 +228,13 @@ class Discussion extends Model return $this->hasMany('Flarum\Core\Models\Post'); } - protected static $visiblePostsScopes = []; - - public static function scopeVisiblePosts($scope) - { - static::$visiblePostsScopes[] = $scope; - } - + /** + * Define the relationship with the discussion's posts, but only ones which + * are visible to the given user. + * + * @param \Flarum\Core\Models\User $user + * @return \Illuminate\Database\Eloquent\Relations\HasMany + */ public function visiblePosts(User $user) { $query = $this->posts(); @@ -275,23 +247,27 @@ class Discussion extends Model } /** - * Define the relationship with the discussion's comments. + * Define the relationship with the discussion's publicly-visible comments. * * @return \Illuminate\Database\Eloquent\Relations\HasMany */ public function comments() { - return $this->posts()->where('type', 'comment')->whereNull('hide_time'); + return $this->visiblePosts(new Guest)->where('type', 'comment'); } /** - * Define the relationship with the discussion's participants. + * Query the discussion's participants (a list of unique users who have + * posted in the discussion). * - * @return \Illuminate\Database\Eloquent\Relations\HasMany + * @return \Illuminate\Database\Eloquent\Builder */ public function participants() { - return User::join('posts', 'posts.user_id', '=', 'users.id')->where('posts.discussion_id', $this->id)->select('users.*')->distinct(); + return User::join('posts', 'posts.user_id', '=', 'users.id') + ->where('posts.discussion_id', $this->id) + ->select('users.*') + ->distinct(); } /** @@ -325,7 +301,7 @@ class Discussion extends Model } /** - * Define the relationship with the discussion's last post's author. + * Define the relationship with the discussion's most recent author. * * @return \Illuminate\Database\Eloquent\Relations\BelongsTo */ @@ -345,35 +321,33 @@ class Discussion extends Model } /** - * Define the relationship with the discussion's state for a particular user. + * Define the relationship with the discussion's state for a particular + * user. * - * @param \Flarum\Core\Models\User $user + * If no user is passed (i.e. in the case of eager loading the 'state' + * relation), then the static `$stateUser` property is used. + * + * @see \Flarum\Core\Models\Discussion::setStateUser() + * + * @param \Flarum\Core\Models\User $user * @return \Illuminate\Database\Eloquent\Relations\HasOne */ public function state(User $user = null) { $user = $user ?: static::$stateUser; - return $this->hasOne('Flarum\Core\Models\DiscussionState')->where('user_id', $user->id); + return $this->hasOne('Flarum\Core\Models\DiscussionState')->where('user_id', $user ? $user->id : null); } /** * Get the state model for a user, or instantiate a new one if it does not * exist. * - * @param \Flarum\Core\Models\User $user + * @param \Flarum\Core\Models\User $user * @return \Flarum\Core\Models\DiscussionState */ public function stateFor(User $user) { - // If the state is already eager-loaded, we'll return that. - // Unfortunately we can't check to see if the user ID is the same, - // because the user may not have a state entry, in which case the loaded - // relation will be null. - if (array_key_exists('state', $this->relations)) { - return $this->relations['state']; - } - $state = $this->state($user)->first(); if (! $state) { @@ -388,10 +362,22 @@ class Discussion extends Model /** * Set the user for which the state relationship should be loaded. * - * @param \Flarum\Core\Models\User $user + * @param \Flarum\Core\Models\User $user */ public static function setStateUser(User $user) { static::$stateUser = $user; } + + /** + * Constrain which posts are visible to a user. + * + * @param callable $scope A callback that applies constraints to the posts + * query. It is passed three parameters: the query builder object, the + * user to constrain posts for, and the discussion instance. + */ + public static function addVisiblePostsScope(callable $scope) + { + static::$visiblePostsScopes[] = $scope; + } } diff --git a/src/Core/Models/DiscussionRenamedPost.php b/src/Core/Models/DiscussionRenamedPost.php index 61bffd57e..a9fb581fa 100755 --- a/src/Core/Models/DiscussionRenamedPost.php +++ b/src/Core/Models/DiscussionRenamedPost.php @@ -1,43 +1,52 @@ user_id === $previous->user_id) { + // If the previous post is another 'discussion renamed' post, and it's + // by the same user, then we can merge this post into it. If we find + // that we've in fact reverted the title, delete it. Otherwise, update + // its content. + if ($previous instanceof static && $this->user_id === $previous->user_id) { if ($previous->content[0] == $this->content[1]) { - return; + $previous->delete(); + } else { + $previous->content = static::buildContent($previous->content[0], $this->content[1]); + + $previous->save(); } - $previous->content = static::buildContent($previous->content[0], $this->content[1]); return $previous; } + $this->save(); + return $this; } /** * Create a new instance in reply to a discussion. * - * @param int $discussionId - * @param int $userId - * @param string $oldTitle - * @param string $newTitle - * @return static + * @param int $discussionId + * @param int $userId + * @param string $oldTitle + * @param string $newTitle + * @return \Flarum\Core\Models\DiscussionRenamedPost */ public static function reply($discussionId, $userId, $oldTitle, $newTitle) { @@ -54,11 +63,11 @@ class DiscussionRenamedPost extends EventPost /** * Build the content attribute. * - * @param boolean $oldTitle The old title of the discussion. - * @param boolean $newTitle The new title of the discussion. + * @param string $oldTitle The old title of the discussion. + * @param string $newTitle The new title of the discussion. * @return array */ - public static function buildContent($oldTitle, $newTitle) + protected static function buildContent($oldTitle, $newTitle) { return [$oldTitle, $newTitle]; } diff --git a/src/Core/Models/EventPost.php b/src/Core/Models/EventPost.php index 6c392b29c..084e2a9be 100755 --- a/src/Core/Models/EventPost.php +++ b/src/Core/Models/EventPost.php @@ -1,13 +1,11 @@