pierobot / mangapie

This is a self-hosted server for archived manga.
BSD 3-Clause "New" or "Revised" License
65 stars 9 forks source link

History is broken #226

Closed kurim closed 3 years ago

kurim commented 3 years ago

If multiple user read the same manga the history is broken.

I think the MySQL Query for this is the issue, in this function:

File: UserController.php public function history(User $user)

The query could be like this, but i have no clue how to get this work in current framework

SELECT DISTINCT
    `page`,
    `page_count`,
    `manga_id`,
    `updated_at`,
    `archive_id`
FROM
    (
    SELECT
        PI.manga_id,
        PI.archive_id,
        PI.page,
        PI.page_count,
        PI.updated_at,
        PI.id
    FROM
        reader_histories PI
    WHERE
        user_id = 3
    ORDER BY
        PI.updated_at
    DESC
LIMIT 30
) T
GROUP BY
    manga_id
ORDER BY
    updated_at
DESC

or as working workaround but with the limitation to disable strict mode:

$items = ReaderHistory::query()->fromraw('( SELECT PI.manga_id, PI.archive_id, PI.page, PI.page_count, PI.updated_at, PI.id FROM reader_histories PI WHERE user_id = '.$user->id.' ORDER BY PI.updated_at DESC LIMIT 30) T')
                ->select(['page', 'page_count', 'manga_id','updated_at','archive_id'])
                ->distinct('manga_id')
                ->groupBy('manga_id')
                ->orderByDesc('updated_at')
                ->with(['manga', 'archive'])
                ->take(30)
                ->get();

to leave stirct mode on requieres to avoid use of GROUP BY, where i currently have no clue how to do it.

Edit: now it works as expected...

pierobot commented 3 years ago

If multiple user read the same manga the history is broken.

Care to elaborate so I can make a test? There was a reason why I did not use groupBy but I cannot remember why right now.

kurim commented 3 years ago

Sure:

if you have two user reading the same manga, the current SQL query mix them up, this ends in the situation that reader one dont get the correct history of his mangas.

Current state: image

And what i read: image

the loaded history is from user 4 image

I know why u did not use groupby, it does not work with strict mode. I found a way to workaround the "group by" you need to use "with"

WITH reader_histories AS (
  SELECT *, ROW_NUMBER() OVER (PARTITION BY manga_id ORDER BY id DESC) AS rn
  FROM reader_histories AS m WHERE user_id = 3
)
SELECT * FROM reader_histories WHERE rn = 1 ORDER BY updated_at DESC

Since laravel does not support it from it self you need to add this: https://github.com/staudenmeir/laravel-cte https://stackoverflow.com/questions/56142900/how-to-use-with-clause-in-laravel-query-builder

//edit: but I'm happy to see that this project is not dead and you are back again 👍

//edit2: ok the laravel-cte does not work because a query w/o group Syntax error or access violation: 1140 Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause, it's annoying that i know that it works in mysql as a normal query...

there is another way for group by https://medium.com/dev-i/using-groupby-method-in-laravel-608cd2449490 But this seems not working with "->with(['manga', 'archive'])"

pierobot commented 3 years ago

I'll start writing the tests.

I've been working a lot and coding on my spare time started feeling like another job so I took a break. I should be getting a long break relatively soon so I'm picking the project back up.

pierobot commented 3 years ago

Well I finally got time but simple tests using factories are all failing... all I did was update some dependencies.

Hopefully updating the framework solves it. Otherwise it might take time to figure out why they are failing

EDIT:

Should be fixed.

kurim commented 3 years ago

hm, is it intended that i now can see every read chapter? not just last one? image