Slug Driver Support (#2456)

- Support slug drivers for core's sluggable models, easily extends to other models - Add automated testing for affected single-model API routes - Fix nickname selection UI - Serialize slugs as `slug` attribute - Make min search length a constant
flarum · Dec 7, 2020 · 4679448 · 4679448
1 parent ef4bf81
commit 4679448
Show file tree

Hide file tree

Showing 27 changed files with 670 additions and 57 deletions.
diff --git a/js/src/admin/components/BasicsPage.js b/js/src/admin/components/BasicsPage.js
@@ -25,10 +25,6 @@ export default class BasicsPage extends Page {
       'welcome_message',
       'display_name_driver',
     ];
-    this.values = {};
-
-    const settings = app.data.settings;
-    this.fields.forEach((key) => (this.values[key] = Stream(settings[key])));
 
     this.localeOptions = {};
     const locales = app.data.locales;
@@ -42,8 +38,29 @@ export default class BasicsPage extends Page {
       this.displayNameOptions[identifier] = identifier;
     }, this);
 
+    this.slugDriverOptions = {};
+    Object.keys(app.data.slugDrivers).forEach((model) => {
+      this.fields.push(`slug_driver_${model}`);
+      this.slugDriverOptions[model] = {};
+
+      app.data.slugDrivers[model].forEach((option) => {
+        this.slugDriverOptions[model][option] = option;
+      });
+    });
+
+    this.values = {};
+
+    const settings = app.data.settings;
+    this.fields.forEach((key) => (this.values[key] = Stream(settings[key])));
+
     if (!this.values.display_name_driver() && displayNameDrivers.includes('username')) this.values.display_name_driver('username');
 
+    Object.keys(app.data.slugDrivers).forEach((model) => {
+      if (!this.values[`slug_driver_${model}`]() && 'default' in this.slugDriverOptions[model]) {
+        this.values[`slug_driver_${model}`]('default');
+      }
+    });
+
     if (typeof this.values.show_language_selector() !== 'number') this.values.show_language_selector(1);
   }
 
@@ -132,20 +149,30 @@ export default class BasicsPage extends Page {
               ]
             )}
 
-            {Object.keys(this.displayNameOptions).length > 1
-              ? FieldSet.component(
-                  {
-                    label: app.translator.trans('core.admin.basics.display_name_heading'),
-                  },
-                  [
-                    <div className="helpText">{app.translator.trans('core.admin.basics.display_name_text')}</div>,
-                    Select.component({
-                      options: this.displayNameOptions,
-                      bidi: this.values.display_name_driver,
-                    }),
-                  ]
-                )
-              : ''}
+            {Object.keys(this.displayNameOptions).length > 1 ? (
+              <FieldSet label={app.translator.trans('core.admin.basics.display_name_heading')}>
+                <div className="helpText">{app.translator.trans('core.admin.basics.display_name_text')}</div>
+                <Select
+                  options={this.displayNameOptions}
+                  value={this.values.display_name_driver()}
+                  onchange={this.values.display_name_driver}
+                ></Select>
+              </FieldSet>
+            ) : (
+              ''
+            )}
+
+            {Object.keys(this.slugDriverOptions).map((model) => {
+              const options = this.slugDriverOptions[model];
+              if (Object.keys(options).length > 1) {
+                return (
+                  <FieldSet label={app.translator.trans('core.admin.basics.slug_driver_heading', { model })}>
+                    <div className="helpText">{app.translator.trans('core.admin.basics.slug_driver_text', { model })}</div>
+                    <Select options={options} value={this.values[`slug_driver_${model}`]()} onchange={this.values[`slug_driver_${model}`]}></Select>
+                  </FieldSet>
+                );
+              }
+            })}
 
             {Button.component(
               {

diff --git a/js/src/common/models/User.js b/js/src/common/models/User.js
@@ -10,6 +10,7 @@ export default class User extends Model {}
 
 Object.assign(User.prototype, {
   username: Model.attribute('username'),
+  slug: Model.attribute('slug'),
   displayName: Model.attribute('displayName'),
   email: Model.attribute('email'),
   isEmailConfirmed: Model.attribute('isEmailConfirmed'),

diff --git a/js/src/forum/components/DiscussionListItem.js b/js/src/forum/components/DiscussionListItem.js
@@ -14,6 +14,7 @@ import DiscussionControls from '../utils/DiscussionControls';
 import slidable from '../utils/slidable';
 import extractText from '../../common/utils/extractText';
 import classList from '../../common/utils/classList';
+import DiscussionPage from './DiscussionPage';
 
 import { escapeRegExp } from 'lodash-es';
 /**
@@ -156,9 +157,7 @@ export default class DiscussionListItem extends Component {
    * @return {Boolean}
    */
   active() {
-    const idParam = m.route.param('id');
-
-    return idParam && idParam.split('-')[0] === this.attrs.discussion.id();
+    return app.current.matches(DiscussionPage, { discussion: this.attrs.discussion });
   }
 
   /**

diff --git a/js/src/forum/components/DiscussionPage.js b/js/src/forum/components/DiscussionPage.js
@@ -109,7 +109,7 @@ export default class DiscussionPage extends Page {
     } else {
       const params = this.requestParams();
 
-      app.store.find('discussions', m.route.param('id').split('-')[0], params).then(this.show.bind(this));
+      app.store.find('discussions', m.route.param('id'), params).then(this.show.bind(this));
     }
 
     m.redraw();
@@ -123,6 +123,7 @@ export default class DiscussionPage extends Page {
    */
   requestParams() {
     return {
+      bySlug: true,
       page: { near: this.near },
     };
   }

diff --git a/js/src/forum/components/Search.js b/js/src/forum/components/Search.js
@@ -21,6 +21,8 @@ import UsersSearchSource from './UsersSearchSource';
  * - state: SearchState instance.
  */
 export default class Search extends Component {
+  static MIN_SEARCH_LEN = 3;
+
   oninit(vnode) {
     super.oninit(vnode);
     this.state = this.attrs.state;
@@ -152,7 +154,7 @@ export default class Search extends Component {
         search.searchTimeout = setTimeout(() => {
           if (state.isCached(query)) return;
 
-          if (query.length >= 3) {
+          if (query.length >= Search.MIN_SEARCH_LEN) {
             search.sources.map((source) => {
               if (!source.search) return;
 

diff --git a/js/src/forum/components/UserPage.js b/js/src/forum/components/UserPage.js
@@ -102,7 +102,7 @@ export default class UserPage extends Page {
     });
 
     if (!this.user) {
-      app.store.find('users', username).then(this.show.bind(this));
+      app.store.find('users', username, { bySlug: true }).then(this.show.bind(this));
     }
   }
 

diff --git a/js/src/forum/resolvers/DiscussionPageResolver.ts b/js/src/forum/resolvers/DiscussionPageResolver.ts
@@ -1,15 +1,6 @@
 import DefaultResolver from '../../common/resolvers/DefaultResolver';
 import DiscussionPage from '../components/DiscussionPage';
 
-/**
- * This isn't exported as it is a temporary measure.
- * A more robust system will be implemented alongside UTF-8 support in beta 15.
- */
-function getDiscussionIdFromSlug(slug: string | undefined) {
-  if (!slug) return;
-  return slug.split('-')[0];
-}
-
 /**
  * A custom route resolver for DiscussionPage that generates the same key to all posts
  * on the same discussion. It triggers a scroll when going from one post to another
@@ -18,17 +9,32 @@ function getDiscussionIdFromSlug(slug: string | undefined) {
 export default class DiscussionPageResolver extends DefaultResolver {
   static scrollToPostNumber: string | null = null;
 
+  /**
+   * Remove optional parts of a discussion's slug to keep the substring
+   * that bijectively maps to a discussion object. By default this just
+   * extracts the numerical ID from the slug. If a custom discussion
+   * slugging driver is used, this may need to be overriden.
+   * @param slug
+   */
+  canonicalizeDiscussionSlug(slug: string | undefined) {
+    if (!slug) return;
+    return slug.split('-')[0];
+  }
+
+  /**
+   * @inheritdoc
+   */
   makeKey() {
     const params = { ...m.route.param() };
     if ('near' in params) {
       delete params.near;
     }
-    params.id = getDiscussionIdFromSlug(params.id);
+    params.id = this.canonicalizeDiscussionSlug(params.id);
     return this.routeName.replace('.near', '') + JSON.stringify(params);
   }
 
   onmatch(args, requestedPath, route) {
-    if (app.current.matches(DiscussionPage) && getDiscussionIdFromSlug(args.id) === getDiscussionIdFromSlug(m.route.param('id'))) {
+    if (app.current.matches(DiscussionPage) && this.canonicalizeDiscussionSlug(args.id) === this.canonicalizeDiscussionSlug(m.route.param('id'))) {
       // By default, the first post number of any discussion is 1
       DiscussionPageResolver.scrollToPostNumber = args.near || '1';
     }

diff --git a/js/src/forum/routes.js b/js/src/forum/routes.js
@@ -34,9 +34,8 @@ export default function (app) {
    * @return {String}
    */
   app.route.discussion = (discussion, near) => {
-    const slug = discussion.slug();
     return app.route(near && near !== 1 ? 'discussion.near' : 'discussion', {
-      id: discussion.id() + (slug.trim() ? '-' + slug : ''),
+      id: discussion.slug(),
       near: near && near !== 1 ? near : undefined,
     });
   };
@@ -59,7 +58,7 @@ export default function (app) {
    */
   app.route.user = (user) => {
     return app.route('user', {
-      username: user.username(),
+      username: user.slug(),
     });
   };
 }
diff --git a/src/Admin/Content/AdminPayload.php b/src/Admin/Content/AdminPayload.php
@@ -75,6 +75,9 @@ public function __invoke(Document $document, Request $request)
         $document->payload['extensions'] = $this->extensions->getExtensions()->toArray();
 
         $document->payload['displayNameDrivers'] = array_keys($this->container->make('flarum.user.display_name.supported_drivers'));
+        $document->payload['slugDrivers'] = array_map(function ($resourceDrivers) {
+            return array_keys($resourceDrivers);
+        }, $this->container->make('flarum.http.slugDrivers'));
 
         $document->payload['phpVersion'] = PHP_VERSION;
         $document->payload['mysqlVersion'] = $this->db->selectOne('select version() as version')->version;

diff --git a/src/Api/Controller/ShowDiscussionController.php b/src/Api/Controller/ShowDiscussionController.php
@@ -12,6 +12,7 @@
 use Flarum\Api\Serializer\DiscussionSerializer;
 use Flarum\Discussion\Discussion;
 use Flarum\Discussion\DiscussionRepository;
+use Flarum\Http\SlugManager;
 use Flarum\Post\PostRepository;
 use Flarum\User\User;
 use Illuminate\Support\Arr;
@@ -31,6 +32,11 @@ class ShowDiscussionController extends AbstractShowController
      */
     protected $posts;
 
+    /**
+     * @var SlugManager
+     */
+    protected $slugManager;
+
     /**
      * {@inheritdoc}
      */
@@ -61,11 +67,13 @@ class ShowDiscussionController extends AbstractShowController
     /**
      * @param \Flarum\Discussion\DiscussionRepository $discussions
      * @param \Flarum\Post\PostRepository $posts
+     * @param \Flarum\Http\SlugManager $slugManager
      */
-    public function __construct(DiscussionRepository $discussions, PostRepository $posts)
+    public function __construct(DiscussionRepository $discussions, PostRepository $posts, SlugManager $slugManager)
     {
         $this->discussions = $discussions;
         $this->posts = $posts;
+        $this->slugManager = $slugManager;
     }
 
     /**
@@ -77,7 +85,11 @@ protected function data(ServerRequestInterface $request, Document $document)
         $actor = $request->getAttribute('actor');
         $include = $this->extractInclude($request);
 
-        $discussion = $this->discussions->findOrFail($discussionId, $actor);
+        if (Arr::get($request->getQueryParams(), 'bySlug', false)) {
+            $discussion = $this->slugManager->forResource(Discussion::class)->fromSlug($discussionId, $actor);
+        } else {
+            $discussion = $this->discussions->findOrFail($discussionId, $actor);
+        }
 
         if (in_array('posts', $include)) {
             $postRelationships = $this->getPostRelationships($include);

diff --git a/src/Api/Controller/ShowUserController.php b/src/Api/Controller/ShowUserController.php
@@ -11,6 +11,8 @@
 
 use Flarum\Api\Serializer\CurrentUserSerializer;
 use Flarum\Api\Serializer\UserSerializer;
+use Flarum\Http\SlugManager;
+use Flarum\User\User;
 use Flarum\User\UserRepository;
 use Illuminate\Support\Arr;
 use Psr\Http\Message\ServerRequestInterface;
@@ -29,15 +31,22 @@ class ShowUserController extends AbstractShowController
     public $include = ['groups'];
 
     /**
-     * @var \Flarum\User\UserRepository
+     * @var SlugManager
+     */
+    protected $slugManager;
+
+    /**
+     * @var UserRepository
      */
     protected $users;
 
     /**
-     * @param \Flarum\User\UserRepository $users
+     * @param SlugManager $slugManager
+     * @param UserRepository $users
      */
-    public function __construct(UserRepository $users)
+    public function __construct(SlugManager $slugManager, UserRepository $users)
     {
+        $this->slugManager = $slugManager;
         $this->users = $users;
     }
 
@@ -47,17 +56,18 @@ public function __construct(UserRepository $users)
     protected function data(ServerRequestInterface $request, Document $document)
     {
         $id = Arr::get($request->getQueryParams(), 'id');
+        $actor = $request->getAttribute('actor');
 
-        if (! is_numeric($id)) {
-            $id = $this->users->getIdForUsername($id);
+        if (Arr::get($request->getQueryParams(), 'bySlug', false)) {
+            $user = $this->slugManager->forResource(User::class)->fromSlug($id, $actor);
+        } else {
+            $user = $this->users->findOrFail($id, $actor);
         }
 
-        $actor = $request->getAttribute('actor');
-
-        if ($actor->id == $id) {
+        if ($actor->id === $user->id) {
             $this->serializer = CurrentUserSerializer::class;
         }
 
-        return $this->users->findOrFail($id, $actor);
+        return $user;
     }
 }
diff --git a/src/Api/Serializer/BasicDiscussionSerializer.php b/src/Api/Serializer/BasicDiscussionSerializer.php
@@ -10,6 +10,7 @@
 namespace Flarum\Api\Serializer;
 
 use Flarum\Discussion\Discussion;
+use Flarum\Http\SlugManager;
 use InvalidArgumentException;
 
 class BasicDiscussionSerializer extends AbstractSerializer
@@ -19,6 +20,16 @@ class BasicDiscussionSerializer extends AbstractSerializer
      */
     protected $type = 'discussions';
 
+    /**
+     * @var SlugManager
+     */
+    protected $slugManager;
+
+    public function __construct(SlugManager $slugManager)
+    {
+        $this->slugManager = $slugManager;
+    }
+
     /**
      * {@inheritdoc}
      *
@@ -35,7 +46,7 @@ protected function getDefaultAttributes($discussion)
 
         return [
             'title' => $discussion->title,
-            'slug'  => $discussion->slug,
+            'slug' =>  $this->slugManager->forResource(Discussion::class)->toSlug($discussion),
         ];
     }
-Original file line number
+Diff line change
@@ Expand Up / @@ -102,7 +102,7 @@ export default class UserPage extends Page { @@
         });
         if (!this.user) {
-          app.store.find('users', username).then(this.show.bind(this));
+          app.store.find('users', username, { bySlug: true }).then(this.show.bind(this));
         }
       }
@@ Expand Down @@