Does it make sens to implement a new query to simplify listening all content?

[513ry]

API Endpoint or Feature

Hey, I made a hack for drawing a tree of content to easily traverse the wiki from the home page. My colleague noticed issue #616 and thought it would be good to work on a more generic and cleaner version of this code to merge with upstream.

I was thinking of expanding the view switch button with a "Tree View"/"Table View" option to integrate this feature and maybe adding a setting to display it in the sidebar as we currently have it. Anyway, I’ve been trying to follow your convention of having this view button for both Book and Bookshelf views.

To simplify the view code, I wrote an additional query that returns either an expanded version of a Book Collection with a contents member or a Collection of Bookshelves expanded by a books member. Here is my initial implementation with some limitations:

<?php

namespace BookStack\Entities\Queries;

use BookStack\Permissions\PermissionApplicator;
use BookStack\Entities\Models\Entity;
use BookStack\Entities\Models\Book;
use BookStack\Entities\Models\Bookshelf;
use Illuminate\Database\Eloquent\Collection;
use InvalidArgumentException;

class QueryUserContent
{
    private function __construct() {}

    /**
     * Return all books available for the current user with 'content' field.
     */
    public static function books(string $sortContentBy = 'id'): ?Collection
    {
        // Return null if no user is logged in
        if (user()->isGuest()) {
            return null;
        }

        $books = self::sortCollection(
            self::filterEntities(Book::class), $sortContentBy
        );
        // Expand books with their contents
        foreach ($books as $id => $book) {
            $directPages =
                ($directPages = $book->directPages) ? self::sortCollection($directPages, $sortContentBy) : [];
            $chapters =
                ($chapters = $book->chapters()->with('pages')->get()) ? self::sortCollection($chapters, $sortContent
By) : [];
            $contents = $chapters->union($directPages);
            $books[$id]['contents'] = $contents;
        }
        return $books;
    }

    /**
     * Return all books avaiable for the current user categorized in shelves
     * with 'books' field.
     *
     * @param string sortShelvesBy - Key by which to sort queried shelves
     * @param string sortContentBy - Key by which to sort queried content
     * @return Collection
     */
    public static function shelves(string $sortShelvesBy = 'id',
                                   string $sortContentBy = 'id'): ?Collection
    {
        $books = self::books($sortContentBy);
        if (!$books) return null;
        $shelves = self::sortCollection(
            self::filterEntities(Bookshelf::class), $sortShelvesBy
        );
        // Expand bookshelves with their books (ignores one to many relation of
        // books to shelves by using the first associated shelf)
        $shelves = $shelves->map(function ($shelf) use ($books) {
            $shelfBooks = $books->filter(fn($book) =>
                $book->shelves instanceof Collection
                && ($firstShelf = $book->shelves->first()) !== null
                && $firstShelf->id === $shelf->id
            );
            $shelf['books'] = $shelfBooks->values();
            return $shelf;
        });
        return $shelves;
    }

    /**
     * Ensure Entity type and return items that the user has view permission for.
     *
     * @param string entityClass - Name of the Entity subclass
     * @return Collection
     */
    private static function filterEntities(string $entityClass): Collection
    {
        if (!is_subclass_of($entityClass, Entity::class)) {
            throw new InvalidArgumentException("$entityClass must be a subclass of Entity");
        }

        $query = $entityClass::query();
        $permissions = new PermissionApplicator();
        $query = $permissions->restrictEntityQuery($query);
        return $query->get();
    }

    /**
     * Sort an Eloquent Collection by key.
     *
     * @return Collection
     */
    private static function sortCollection(Collection $collection, string $key): Collection
    {
        if ($collection->isEmpty()) {
            return $collection;
        }
        if (!array_key_exists($key, $collection->first()->getAttributes())) {
            throw new InvalidArgumentException(
                "Expected second argument to be one of Collection's keys but get \"$key\""
            );
        }

        return $collection->sortBy($key)->values();
    }
}

Any criticism is welcome since I just picked up PHP this week. I think the comments explain some of the limitations of this code so far.

However, I noticed that this will probably create a lot of overhead, given that you already list shelves and books accordingly when checking homepage options in the home controller:

if ($homepageOption === 'bookshelves') {
  $shelves = $this->queries->shelves->visibleForListWithCover()
    ->orderBy($commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder())
    ->paginate(18);
  $data = array_merge($commonData, ['shelves' => $shelves]);

  return view('home.shelves', $data);
}

if ($homepageOption === 'books') {
  $books = $this->queries->books->visibleForListWithCover()
    ->orderBy($commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder())
    ->paginate(18);
  $data = array_merge($commonData, ['books' => $books]);

  return view('home.books', $data);
}

I wonder now - should I do this work of associating contents with books and books with shelves inside my partial element code, or do you see a use for this query to reduce redundancy elsewhere? In the latter case, it would make sense to replace the home controller’s $books and $shelves with objects returned by this query.

Use-Case

Implementing solution to #616.

Additional context

No response

[ssddanbrown]

Hi @513ry,
Thanks for offering, but I don't really see that existing code as redundant as it's a bit different to what you're getting via that class, and since that existing query logic is within its own class of queries that are used elsewhere.

BTW, If it's important to your use-case, I'm not sure your class is considering permissions/visibility for pages/chapters.
There is an example of book tree building here.

[513ry]

Since there is already this nice tool which solves the problem of sorting book contents for a tree, maybe the better solution would be to use this tool to implement similar thing for bookshelves. What do you think about this?

[ssddanbrown]

We don't have a need in the core codebase for that right now so it's not something I'd look to add.
Use would direct implementation, and we don't have a use for that at the moment.

[513ry]

Should I then restrict the implementation to the controller for the tree view partial?

Do you intend thought to merge #616 with the core codebase? A lot of people seem to want this.

[ssddanbrown]

Should I then restrict the implementation to the controller for the tree view partial?

Depends on your customization I guess. If this was officially done, I'd still have a builder class which is then used in that controller method, although some refactoring of the home route handling might be warranted upon further growth.

Do you intend thought to merge #616 with the core codebase?

No current intent, but I'm not specifically against the idea, just needs some consideration/evaluation to confirm the idea before going to any kind of implementation stage.
I'll add a comment there to start this process for discussion/feedback.