fix duplication of solutions, issue #1681 #1738

rnik82 · 2025-09-24T18:03:12Z

Pull request details

Removed the duplication of the solution on the code review page and fixed the solution comparison mechanism.
For guests added "Log in to see the solution" stub.

Issues fixed

Fix two issues #1681 and #1766

Link to demo

https://hexlet-sicp-449a.onrender.com

fey · 2025-09-30T16:14:53Z

resources/views/solution/show.blade.php

    </div>

-    @if (count($solutionsListForCurrentExercise) > 1)
+    @if (count($solutionsOfOtherUsers ?? []) > 0)


в solutionsOfOtherUsers у тебя в любом случае должен быть определени, тк там из базы берется, то будет eloquent collection, на которой можно вызвать метод проверки на пустоту.

fey · 2025-09-30T16:20:37Z

resources/views/solution/show.blade.php

        </ul>
        <div class="tab-content" id="pills-tabContent">
-          @foreach ($solutionsListForCurrentExercise as $currentSolution)
+          @foreach ($solutionsOfOtherUsers as $currentSolution)


тут в двух местах переименовывается переменая. Но, хм странно. Ведь получается что в solutionsListForCurrentExercise - решения пользователя, у которого страницу открываем users/1/solutions - для id 1, а в other - для всех остальных. Получается, что нам нужны обе переменных, ведь мы сравниваем решение текущего пользователя и другого.

Изначально там было два одинаковых блока с solutionsListForCurrentExercise, поэтому и дублировались решения юзера и собстенно в этом была ошибка. А я во втором блоке заменил solutionsListForCurrentExercise на solutionsOfOtherUsers, чтобы вначале показывалось решение юзера, а следом в следующем блоке решения других пользователей.
Тут в дифе показаны только мои изменения во втором блоке и не виден первый блок с solutionsListForCurrentExercise, потому что я его не трогал.

fey · 2025-09-30T16:25:01Z

@rnik82 давай попробуем улучшить текущую страничку. Сейчас у нас два таба и идея была в том, чтобы сравнить два решения. Свое и чужое.
Соответственно так и стоит сравнивать, - выводить решение текущего пользователя на текущей странице (допустим, слева), выводить справа - наше решение. Если мы решение для этого задания не сохраняли, можно вывести текст типа "У вас нет сохраненных решений для этого задания". Если пользователь смотрит свои решения, то просто не выводим второй таб.

Сейчас получается при любых раскладах у тебя не выводится второй таб https://hexlet-sicp-449a.onrender.com/solutions/28
https://hexlet-sicp-449a.onrender.com/solutions/31

rnik82 · 2025-10-01T15:05:23Z

Сейчас получается при любых раскладах у тебя не выводится второй таб https://hexlet-sicp-449a.onrender.com/solutions/28 https://hexlet-sicp-449a.onrender.com/solutions/31

@fey, странно. У меня показывает решения и юзера и других пользователей если они есть (см скрин)

Просто они сейчас одно под другим, а нужно, как я понял, сделать одно слева, другое справа. И еще решения других юзеров сейчас промаркированы как v.1, v.2, v.3, v.4, v.5, v.6 и тд. При этом решения, условно, Васи это v.1 и v.2, решения Пети это v.3, v.4, v.5, а решения Вани это v.6.

Вопрос: как лучше назвать решения других польователей? Потому что v.1, v.2, v.3... как то не очень. Может просто 1, 2, 3...?

Если мы решение для этого задания не сохраняли, можно вывести текст типа "У вас нет сохраненных решений для этого задания".

Сейчас если мы заходим в свои решения, то там показаны только те, которые мы сохранили. То есть нам просто не показывает в списке никаких других решений, в том числе тех, которые мы прорешали но не сохранили. То есть ты предлагаешь выводить в списке все, которые мы решили, не важно сохраняли или нет. Я правильно понял?

Если пользователь смотрит свои решения, то просто не выводим второй таб.

То есть тут будет не как на Хекслете, а как-то по другому? Там всегда показывает "Ваше решение" (можно пощелкать разные версии), а рядом всегда "Решение учителя". Вроде там не было такого, что ты смотришь только свои решения без решений учителя.

fey · 2025-10-02T05:56:09Z

Сейчас если мы заходим в свои решения, то там показаны только те, которые мы сохранили. То есть нам просто не показывает в списке никаких других решений, в том числе тех, которые мы прорешали но не сохранили. То есть ты предлагаешь выводить в списке все, которые мы решили, не важно сохраняли или нет. Я правильно понял?

не, если мы сохранили это все верно. Ведь мы не можем показать то, что не сохранили

Просто они сейчас одно под другим, а нужно, как я понял, сделать одно слева, другое справа.

Да, стоит сделать слева и справа, чтобы было удобно сравнивать. А сверху-снизу для мобилок/узких экранов.

То есть тут будет не как на Хекслете, а как-то по другому? Там всегда показывает "Ваше решение" (можно пощелкать разные версии), а рядом всегда "Решение учителя". Вроде там не было такого, что ты смотришь только свои решения без решений учителя.

Можно выводить решение учителя, но это уже можно второй версией (след пр).

fey · 2025-10-02T06:04:14Z

@fey, странно. У меня показывает решения и юзера и других пользователей если они есть (см скрин)

Открываю https://hexlet-sicp-449a.onrender.com/ru/solutions/24

Проверь, что у тебя задеплоена актуальная версия.

rnik82 · 2025-10-02T09:57:11Z

@fey, там оказалось что есть два обработчика show, которые используют один и тот же шаблон:
https://github.com/Hexlet/hexlet-sicp/blob/main/app/Http/Controllers/SolutionController.php
и
https://github.com/Hexlet/hexlet-sicp/blob/main/app/Http/Controllers/User/SolutionController.php

Соответственно, если мы заходим, например, сюда - https://hexlet-sicp-449a.onrender.com/ru/solutions/31
то видим только Свое решение.

А если сюда - https://hexlet-sicp-449a.onrender.com/ru/users/11/solutions/31,
то видим свое и чужие, если они есть.

По хорошему, тут, наверное, два отдельных шаблона нужно делать.

Я там вчера немного изменил сам шаблон, и сделал две колонки. Код только что обновил на рендере.

rnik82 · 2025-10-03T10:41:44Z

@fey, наверное, без твоего мнения не обойтись.

Как я уже писал, у нас есть два обработчика, два маршрута и один шаблон.

Вот первый вариант зайти на страницу Код ревью:

Ну далее нужно провалиться в чье-то решение.

Вот второй:
Иконка человека сверху справа -> Прогресс -> Мои решения -> Далее выбрать свое сохраненное решение

Мое предложение, разделить шаблоны.
В первом случае убрать слова "Код Ревью", "Сравни свои решения", а вместо этого написать, например, "Решение" и показывать только это конкретное выбранное решение без всякого сравнения.
Во втором случае, когда мы заходим через Прогресс, оставить все как есть. Я там сделал две колонки, слева - разные версии наших решений, справа - чужие для сравнения.

Что скажешь? Или тут была совсем другая идея?

fey · 2025-10-03T11:10:22Z

Что скажешь? Или тут была совсем другая идея?

Давай разделим. Чемто не помню какая идея была. Может быть в ищщусаъ/ПРах есть комменты.
Точно была идея сравнивать свое решение и чужое.

fey · 2025-10-03T13:52:55Z

app/Http/Controllers/User/SolutionController.php

-        $this->authorizeResource(Solution::class, 'solution');
+        $this->authorizeResource(Solution::class, 'solution', [
+            'except' => ['show'],
+        ]);


давай сделаем здесь через only. Белые списки проще менеджерить. И если появится кастомный не ресурсный роут, то он по дефолту будет закрыт.

Делаю 'only' => ['store', 'destroy'], вместо 'except' => ['show'] и опять получаю ошибку 403 This action is unauthorized. Хз в чем дело.

@fey, если нужно чтобы все работало через $this->authorizeResource в конструкторе, то проще оставить 'except' => ['show']. Если мы добавляем 'only' => ['store', 'destroy'], то не работает, так как у нас больше одного параметра в методах store и destroy (помимо solution еще есть user). Либо можно вручную в этих методах policy подключить - $this->authorize('create', Solution::class) и $this->authorize('delete', $solution). Тогда тогда тоже работает норм. Или может подскажешь что-то еще?

fey · 2025-10-03T13:54:59Z

app/Http/Controllers/User/SolutionController.php

-        return view('solution.show', compact(
+        $solutionsOfOtherUsers = $solution->exercise
+            ->solutions()
+            ->where('user_id', '!=', $user->id)


можно сразу whereNot использовать https://laravel.com/docs/12.x/queries#where-not-clauses

fey · 2025-10-03T13:55:53Z

app/Http/Controllers/User/SolutionController.php

-
        $currentExercise = $solution->exercise;

        $solutionsListForCurrentExercise = $solution->exercise


давай вот эту переменную переименуем сразу, тк у нас появляется две переменных с разным смыслом.
Ведь здесь не все решения для текущ упражнения.

fey · 2025-10-03T13:58:56Z

tests/Feature/Http/Controllers/User/SolutionControllerTest.php

+            $otherSolutionIds = $otherSolutions->pluck('id')->sort()->values()->all();
+
+            return $solutionIds === $otherSolutionIds;
+        });


Давай уберем провери с whereHas. Без них наше тесты более гибкие, нам достаточно просто что ответ ok. А если нужно что-то проверять на странице, это можно делать системными тестами (браузерными).

@fey так то, в принципе, там уже есть такой тест:

public function testShow(): void { $solution = $this->user->solutions()->first(); $response = $this->get(route('users.solutions.show', [$this->user, $solution])); $response->assertOk(); }

Тогда мой вообще не нужен?

Давай, если так считаеш)

fey · 2025-11-06T15:25:21Z

@rnik82 можешь зарезолвить ветки, где поправил. И почистить от закомментированного кода, потом пингануть снова, я еще раз гляну.

rnik82 marked this pull request as draft September 24, 2025 18:41

rnik82 marked this pull request as ready for review September 30, 2025 12:02

rnik82 force-pushed the fix_issue#1681 branch from 1aa762f to f1a45ce Compare September 30, 2025 15:36

fey reviewed Sep 30, 2025

View reviewed changes

rnik82 marked this pull request as draft October 1, 2025 16:25

rnik82 force-pushed the fix_issue#1681 branch 3 times, most recently from 974af5a to 20a0881 Compare October 1, 2025 17:33

rnik82 force-pushed the fix_issue#1681 branch 3 times, most recently from 32e682a to a70a51a Compare October 3, 2025 13:30

rnik82 marked this pull request as ready for review October 3, 2025 13:50

fey reviewed Oct 3, 2025

View reviewed changes

rnik82 force-pushed the fix_issue#1681 branch 2 times, most recently from 0a80bb7 to 95f3349 Compare October 7, 2025 09:59

fix duplication of solution versions and add comparison solutions

6862865

rnik82 force-pushed the fix_issue#1681 branch from 95f3349 to 6862865 Compare October 7, 2025 11:28


		$currentExercise = $solution->exercise;

		$solutionsListForCurrentExercise = $solution->exercise

fix duplication of solutions, issue #1681 #1738

Are you sure you want to change the base?

fix duplication of solutions, issue #1681 #1738

Uh oh!

Conversation

rnik82 commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request details

Issues fixed

Link to demo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fey commented Sep 30, 2025

Uh oh!

rnik82 commented Oct 1, 2025

Uh oh!

fey commented Oct 2, 2025

Uh oh!

fey commented Oct 2, 2025

Uh oh!

rnik82 commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnik82 commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fey commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fey commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rnik82 commented Sep 24, 2025 •

edited

Loading

rnik82 commented Oct 2, 2025 •

edited

Loading

rnik82 commented Oct 3, 2025 •

edited

Loading