Update comment line reference to include old file and sha (#543)

assets/ts/controllers/review_controller.ts

@@ -17,10 +17,13 @@ export default class extends Controller {
         this.commentService
             .getAddCommentForm(
                 this.addCommentUrlValue,
-                DataSet.string(this.revisionFileTarget, 'file'),
+                DataSet.string(this.revisionFileTarget, 'oldPath'),
+                DataSet.string(this.revisionFileTarget, 'newPath'),
                 DataSet.int(line, 'line'),
                 DataSet.int(line, 'lineOffset'),
-                DataSet.int(line, 'lineAfter')
+                DataSet.int(line, 'lineAfter'),
+                DataSet.string(this.revisionFileTarget, 'headSha'),
+                DataSet.string(line, 'lineState')
             )
             .then(form => {
                 this.commentFormTargets.forEach(el => el.remove());

assets/ts/service/CommentService.ts

@@ -11,9 +11,9 @@ export default class CommentService {
             .then((response) => response.data);
     }
 
-    public getAddCommentForm(url: string, filePath: string, line: number, offset: number, lineAfter: number): Promise<HTMLElement> {
+    public getAddCommentForm(url: string, oldPath: string, newPath: string, line: number, offset: number, lineAfter: number, headSha: string, state: string): Promise<HTMLElement> {
         return this.client
-            .get(url, {params: {filePath, line, offset, lineAfter}})
+            .get(url, {params: {oldPath, newPath, line, offset, lineAfter, state, headSha}})
             .then(response => response.data)
             .then(html => Elements.create(html));
     }

config/packages/framework.php

@@ -9,4 +9,5 @@
     $framework->httpMethodOverride(true);
     $framework->phpErrors()->log(true)->throw(true);
     $framework->handleAllThrowables(true);
+    $framework->annotations()->enabled(false);
 };

migrations/Version20231215093301.php

@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DoctrineMigrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Auto-generated Migration: Please modify to your needs!
+ */
+final class Version20231215093301 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return '';
+    }
+
+    public function up(Schema $schema): void
+    {
+        // this up() migration is auto-generated, please modify it to your needs
+        $this->addSql('ALTER TABLE comment CHANGE line_reference line_reference VARCHAR(2000) NOT NULL');
+    }
+
+    public function down(Schema $schema): void
+    {
+        // this down() migration is auto-generated, please modify it to your needs
+        $this->addSql('ALTER TABLE comment CHANGE line_reference line_reference VARCHAR(500) NOT NULL');
+    }
+}

src/Controller/App/Review/Comment/AddCommentController.php

@@ -11,6 +11,7 @@
 use DR\Review\Repository\Review\CommentRepository;
 use DR\Review\Security\Role\Roles;
 use DR\Review\Service\CodeReview\Comment\CommentEventMessageFactory;
+use DR\Utils\Assert;
 use Symfony\Bridge\Doctrine\Attribute\MapEntity;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\Request;
@@ -47,7 +48,7 @@ public function __invoke(Request $request, #[MapEntity] CodeReview $review): Jso
         $comment = new Comment();
         $comment->setUser($user);
         $comment->setReview($review);
-        $comment->setFilePath($lineReference->filePath);
+        $comment->setFilePath(Assert::notNull($lineReference->oldPath ?? $lineReference->newPath));
         $comment->setLineReference($lineReference);
         $comment->setMessage($data['message']);
         $comment->setCreateTimestamp(time());

src/Entity/Review/Comment.php

@@ -23,7 +23,7 @@ class Comment
     #[ORM\Column(type: 'string', length: 500)]
     private string $filePath;
 
-    #[ORM\Column(type: 'string', length: 500)]
+    #[ORM\Column(type: 'string', length: 2000)]
     private string $lineReference;
 
     // todo change to CommentStateType.

src/Entity/Review/LineReference.php

@@ -9,24 +9,46 @@
 class LineReference implements Stringable
 {
     public function __construct(
-        public readonly string $filePath = '',
+        public readonly ?string $oldPath = null,
+        public readonly ?string $newPath = null,
         public readonly int $line = 1,
         public readonly int $offset = 0,
-        public readonly int $lineAfter = 1
+        public readonly int $lineAfter = 1,
+        public readonly ?string $headSha = null,
+        public readonly LineReferenceStateEnum $state = LineReferenceStateEnum::Unknown
     ) {
     }
 
     public static function fromString(string $reference): LineReference
     {
-        if (preg_match('/^(.*):(\d+):(\d+):(\d+)$/', $reference, $matches) !== 1) {
-            throw new InvalidArgumentException('Invalid reference: ' . $reference);
+        if (preg_match('/^(.*?):(.*?):(\d+):(\d+):(\d+):(\w*):(.)$/', $reference, $matches) === 1) {
+            $oldPath = $matches[1] === '' ? null : $matches[1];
+            $newPath = $matches[2] === '' ? null : $matches[2];
+            $headSha = $matches[6] === '' ? null : $matches[6];
+            $state   = LineReferenceStateEnum::from($matches[7]);
+
+            return new LineReference($oldPath, $newPath, (int)$matches[3], (int)$matches[4], (int)$matches[5], $headSha, $state);
+        }
+
+        if (preg_match('/^(.*):(\d+):(\d+):(\d+)$/', $reference, $matches) === 1) {
+            // backwards compat line reference (file:line:offset:lineAfter)
+            return new LineReference(null, $matches[1], (int)$matches[2], (int)$matches[3], (int)$matches[4], null, LineReferenceStateEnum::Unknown);
         }
 
-        return new LineReference($matches[1], (int)$matches[2], (int)$matches[3], (int)$matches[4]);
+        throw new InvalidArgumentException('Invalid reference: ' . $reference);
     }
 
     public function __toString(): string
     {
-        return sprintf('%s:%d:%d:%d', $this->filePath, $this->line, $this->offset, $this->lineAfter);
+        return sprintf(
+            '%s:%s:%d:%d:%d:%s:%s',
+            $this->oldPath,
+            $this->newPath,
+            $this->line,
+            $this->offset,
+            $this->lineAfter,
+            $this->headSha,
+            $this->state->value
+        );
     }
 }

src/Entity/Review/LineReferenceStateEnum.php

@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DR\Review\Entity\Review;
+
+enum LineReferenceStateEnum: string
+{
+    case Modified = 'M';
+    case Unmodified = 'U';
+    case Added = 'A';
+    case Deleted = 'D';
+    case Unknown = '?';
+
+    /**
+     * @return string[]
+     */
+    public static function values(): array
+    {
+        return array_map(static fn(LineReferenceStateEnum $value): string => $value->value, self::cases());
+    }
+}

src/Request/Comment/AddCommentRequest.php

@@ -6,16 +6,23 @@
 use DigitalRevolution\SymfonyRequestValidation\AbstractValidatedRequest;
 use DigitalRevolution\SymfonyRequestValidation\ValidationRules;
 use DR\Review\Entity\Review\LineReference;
+use DR\Review\Entity\Review\LineReferenceStateEnum;
 
 class AddCommentRequest extends AbstractValidatedRequest
 {
     public function getLineReference(): LineReference
     {
+        $oldPath = $this->request->query->getString('oldPath');
+        $newPath = $this->request->query->getString('newPath');
+
         return new LineReference(
-            (string)$this->request->query->get('filePath'),
+            $oldPath === '' ? null : $oldPath,
+            $newPath === '' ? null : $newPath,
             $this->request->query->getInt('line'),
             $this->request->query->getInt('offset'),
             $this->request->query->getInt('lineAfter'),
+            $this->request->query->getString('headSha'),
+            LineReferenceStateEnum::from($this->request->query->getString('state')),
         );
     }
 
@@ -24,10 +31,13 @@ protected function getValidationRules(): ?ValidationRules
         return new ValidationRules(
             [
                 'query' => [
-                    'filePath'  => 'required|string|filled',
+                    'oldPath'   => 'required|string',
+                    'newPath'   => 'required|string',
                     'line'      => 'required|integer:min:1',
                     'offset'    => 'required|integer:min:0',
-                    'lineAfter' => 'required|integer:min:1'
+                    'lineAfter' => 'required|integer:min:1',
+                    'headSha'   => 'required|string|filled',
+                    'state'     => 'required|in:' . implode(',', LineReferenceStateEnum::values())
                 ]
             ]
         );

src/Service/CodeReview/Comment/CommentEventMessageFactory.php

@@ -19,7 +19,7 @@ public function createAdded(Comment $comment, User $user): CommentAdded
             (int)$comment->getReview()->getId(),
             (int)$comment->getId(),
             $user->getId(),
-            $comment->getLineReference()->filePath,
+            $comment->getFilePath(),
             $comment->getMessage()
         );
     }
@@ -30,7 +30,7 @@ public function createUpdated(Comment $comment, User $user, string $originalComm
             (int)$comment->getReview()->getId(),
             (int)$comment->getId(),
             $user->getId(),
-            $comment->getLineReference()->filePath,
+            $comment->getFilePath(),
             $comment->getMessage(),
             $originalComment
         );
@@ -42,7 +42,7 @@ public function createResolved(Comment $comment, User $user): CommentResolved
             (int)$comment->getReview()->getId(),
             (int)$comment->getId(),
             $user->getId(),
-            $comment->getLineReference()->filePath,
+            $comment->getFilePath(),
         );
     }
 
@@ -52,7 +52,7 @@ public function createUnresolved(Comment $comment, User $user): CommentUnresolve
             (int)$comment->getReview()->getId(),
             (int)$comment->getId(),
             $user->getId(),
-            $comment->getLineReference()->filePath,
+            $comment->getFilePath(),
         );
     }
 
@@ -62,7 +62,7 @@ public function createRemoved(Comment $comment, User $user): CommentRemoved
             (int)$comment->getReview()->getId(),
             (int)$comment->getId(),
             $user->getId(),
-            $comment->getLineReference()->filePath,
+            $comment->getFilePath(),
             $comment->getMessage(),
         );
     }

src/ViewModel/App/Review/FileDiffViewModel.php

@@ -8,6 +8,7 @@
 use DR\Review\Model\Review\Highlight\HighlightedFile;
 use DR\Review\ViewModel\App\Comment\CommentsViewModel;
 use DR\Review\ViewModel\App\Comment\ReplyCommentViewModel;
+use DR\Utils\Arrays;
 
 class FileDiffViewModel
 {
@@ -80,6 +81,11 @@ public function getRevisions(): array
         return $this->revisions;
     }
 
+    public function getHeadSha(): ?string
+    {
+        return Arrays::lastOrNull($this->revisions)?->getCommitHash();
+    }
+
     /**
      * @param Revision[] $revisions
      */

src/ViewModelProvider/CommentViewModelProvider.php

@@ -3,49 +3,23 @@
 
 namespace DR\Review\ViewModelProvider;
 
-use DR\Review\Entity\Git\Diff\DiffFile;
-use DR\Review\Entity\Review\CodeReview;
-use DR\Review\Entity\Review\LineReference;
-use DR\Review\Form\Review\AddCommentFormType;
 use DR\Review\Form\Review\AddCommentReplyFormType;
 use DR\Review\Form\Review\EditCommentFormType;
 use DR\Review\Form\Review\EditCommentReplyFormType;
-use DR\Review\Model\Review\Action\AddCommentAction;
 use DR\Review\Model\Review\Action\AddCommentReplyAction;
 use DR\Review\Model\Review\Action\EditCommentAction;
 use DR\Review\Model\Review\Action\EditCommentReplyAction;
-use DR\Review\Service\CodeReview\DiffFinder;
-use DR\Review\ViewModel\App\Comment\AddCommentViewModel;
 use DR\Review\ViewModel\App\Comment\EditCommentReplyViewModel;
 use DR\Review\ViewModel\App\Comment\EditCommentViewModel;
 use DR\Review\ViewModel\App\Comment\ReplyCommentViewModel;
-use DR\Utils\Assert;
 use Symfony\Component\Form\FormFactoryInterface;
 
 class CommentViewModelProvider
 {
-    public function __construct(private readonly FormFactoryInterface $formFactory, private readonly DiffFinder $diffFinder)
+    public function __construct(private readonly FormFactoryInterface $formFactory)
     {
     }
 
-    public function getAddCommentViewModel(CodeReview $review, DiffFile $file, AddCommentAction $action): AddCommentViewModel
-    {
-        $lineReference = $action->lineReference;
-        $line          = Assert::notNull($this->diffFinder->findLineInFile($file, $lineReference));
-
-        // update lineReference with the diff file original file
-        $lineReference = new LineReference(
-            (string)($file->filePathBefore ?? $file->filePathAfter),
-            $lineReference->line,
-            $lineReference->offset,
-            $lineReference->lineAfter
-        );
-
-        $form = $this->formFactory->create(AddCommentFormType::class, null, ['review' => $review, 'lineReference' => $lineReference])->createView();
-
-        return new AddCommentViewModel($form, $line);
-    }
-
     public function getEditCommentViewModel(EditCommentAction $action): ?EditCommentViewModel
     {
         $comment = $action->comment;

src/ViewModelProvider/Mail/MailCommentViewModelProvider.php

@@ -6,7 +6,6 @@
 use DR\Review\Entity\Review\CodeReview;
 use DR\Review\Entity\Review\Comment;
 use DR\Review\Entity\Review\CommentReply;
-use DR\Review\Entity\Review\LineReference;
 use DR\Review\Entity\User\User;
 use DR\Review\Service\CodeReview\CodeReviewRevisionService;
 use DR\Review\Service\CodeReview\DiffFinder;
@@ -35,16 +34,15 @@ public function createCommentViewModel(
         ?CommentReply $reply = null,
         ?User $resolvedBy = null
     ): CommentViewModel {
-        /** @var LineReference $lineReference */
         $lineReference = $comment->getLineReference();
         $revisions     = $this->revisionService->getRevisions($review);
         $files         = $this->diffService->getDiffForRevisions(Assert::notNull($review->getRepository()), $revisions);
 
         // find selected file
-        $selectedFile = $this->diffFinder->findFileByPath($files, $lineReference->filePath);
+        $selectedFile = $this->diffFinder->findFileByPath($files, $comment->getFilePath());
         $lineRange    = [];
         if ($selectedFile !== null) {
-            $lineRange = $this->diffFinder->findLinesAround($selectedFile, Assert::notNull($lineReference), 6) ?? [];
+            $lineRange = $this->diffFinder->findLinesAround($selectedFile, $lineReference, 6) ?? [];
         }
 
         $headerTitle = $this->getHeaderTitle($comment, $reply, $resolvedBy);

templates/app/review/commit/commit.file.html.twig

@@ -10,7 +10,10 @@
 
     <div class="diff-file__file-revision-scrollpane" {{ stimulus_controller('scroll-positioner') }}>
         <div class="diff-file__file-revision-file" {{ stimulus_target('review', 'revisionFile') }}
-             data-file="{{ file.filePathAfter ?? file.filePathBefore }}">
+             data-old-path="{{ file.filePathBefore }}"
+             data-new-path="{{ file.filePathAfter }}"
+             data-head-sha="{{ fileDiffViewModel.headSha }}"
+        >
             {%- if file.blocks -%}
                 {%- for detachedComment in commentsViewModel.detachedComments -%}
                     {%- include 'app/review/comment/comment.html.twig' with {comment: detachedComment, detached: true, visible: commentsViewModel.isCommentVisible(detachedComment)} -%}

templates/app/review/commit/commit.line.html.twig

@@ -8,7 +8,9 @@
      data-role="diff-line"
      data-line="{{ line_number }}"
      data-line-offset="{{ line_offset }}"
-     data-line-after="{{ line.lineNumberAfter|default('0') }}">
+     data-line-after="{{ line.lineNumberAfter|default('0') }}"
+     data-line-state="{{ macro.line_state(line) }}"
+>
     {%- set codeQualityViewModel = fileDiffViewModel.codeQualityViewModel -%}
 
     {#- gutter -#}

templates/app/review/commit/commit.line.macros.html.twig

@@ -23,6 +23,18 @@
     {{- line_visible ? '' : ' diff-file__diff-line-hidden' -}}
 {%- endmacro -%}
 
+{%- macro line_state(line) -%}
+    {%- if line.state == 1 -%}
+        A
+    {%- elseif line.state == 2 -%}
+        D
+    {%- elseif line.state == 3 -%}
+        M
+    {%- else -%}
+        U
+    {%- endif -%}
+{%- endmacro -%}
+
 {%- macro gutter_double(line, lineNrLengthBefore, lineNrLengthAfter) -%}
     {%- apply spaceless -%}
         <span class="diff-file__line-gutter" data-action="click->review#addComment">