Skip to content

Commit 9476979

Browse files
nunoplopesnunoplopes
committed
fix #34: patch page should be a conversation
1 parent be40c94 commit 9476979

File tree

7 files changed

+159
-55
lines changed

7 files changed

+159
-55
lines changed

‎entities/Patch.php

+25-18
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,11 @@
99
use Doctrine\ORM\Mapping\GeneratedValue;
1010
use Doctrine\ORM\Mapping\Id;
1111
use Doctrine\ORM\Mapping\InheritanceType;
12+
use Doctrine\ORM\Mapping\JoinColumn;
1213
use Doctrine\ORM\Mapping\ManyToOne;
1314
use Doctrine\ORM\Mapping\ManyToMany;
15+
use Doctrine\ORM\Mapping\OneToMany;
16+
use Doctrine\ORM\Mapping\OrderBy;
1417
use Doctrine\ORM\Mapping\Table;
1518
use Doctrine\ORM\Mapping\UniqueConstraint;
1619

@@ -47,7 +50,9 @@ abstract class Patch
4750
/** @Id @Column @GeneratedValue */
4851
public int $id;
4952

50-
/** @ManyToOne(inversedBy="patches") */
53+
/** @ManyToOne(inversedBy="patches")
54+
* @JoinColumn(nullable=false)
55+
*/
5156
public ProjGroup $group;
5257

5358
/** @Column */
@@ -59,18 +64,14 @@ abstract class Patch
5964
/** @Column */
6065
public string $issue_url = 'https://...';
6166

62-
/** @Column(length=2000) */
63-
public string $description;
64-
65-
/** @Column(length=1000) */
66-
public string $review = '';
67+
/** @OneToMany(targetEntity="PatchComment", mappedBy="patch", cascade={"persist"})
68+
* @OrderBy({"id" = "ASC"})
69+
*/
70+
public $comments;
6771

6872
/** @ManyToMany(targetEntity="User") */
6973
public $students;
7074

71-
/** @ManyToOne */
72-
public User $submitter;
73-
7475
/** @Column */
7576
public int $lines_added;
7677

@@ -89,11 +90,13 @@ static function factory(ProjGroup $group, string $url, $type,
8990
throw new ValidationException('Group has no repository yet');
9091

9192
$p = GitHub\GitHubPatch::construct($url, $repo);
92-
$p->group = $group;
93-
$p->type = (int)$type;
94-
$p->issue_url = check_url($issue_url);
95-
$p->description = trim($description);
96-
$p->submitter = $submitter;
93+
$p->group = $group;
94+
$p->type = (int)$type;
95+
$p->issue_url = check_url($issue_url);
96+
97+
$description = trim($description);
98+
$p->comments->add(
99+
new PatchComment($p, "Patch submitted\n$description", $submitter));
97100

98101
try {
99102
$p->updateStats();
@@ -102,7 +105,7 @@ static function factory(ProjGroup $group, string $url, $type,
102105
}
103106

104107
try {
105-
if (!$p->description)
108+
if (!$description)
106109
throw new ValidationException("Empty description");
107110

108111
if (empty($p->students))
@@ -175,13 +178,15 @@ static function factory(ProjGroup $group, string $url, $type,
175178
} catch (ValidationException $ex) {
176179
if (!$ignore_errors)
177180
throw $ex;
178-
$p->description .= "\n\nFailed validation:\n" . $ex->getMessage();
181+
$p->comments->add(
182+
new PatchComment($p, "Failed validation:\n" . $ex->getMessage()));
179183
}
180184

181185
return $p;
182186
}
183187

184188
public function __construct() {
189+
$this->comments = new \Doctrine\Common\Collections\ArrayCollection();
185190
$this->students = new \Doctrine\Common\Collections\ArrayCollection();
186191
}
187192

@@ -305,6 +310,10 @@ public function wasMerged() {
305310
$this->status == PATCH_MERGED_ILLEGAL;
306311
}
307312

313+
public function getSubmitter() : User {
314+
return $this->comments[0]->user;
315+
}
316+
308317
public function set_status($status) {
309318
$status = (int)$status;
310319
if (!isset(self::get_status_options()[$status]))
@@ -320,6 +329,4 @@ public function set_type($type) {
320329
}
321330

322331
public function set_issue_url($url) { $this->issue_url = check_url($url); }
323-
public function set_description($txt) { $this->description = $txt; }
324-
public function set_review($txt) { $this->review = $txt; }
325332
}

‎entities/PatchComment.php

+38
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?php
2+
// Copyright (c) 2022-present Instituto Superior Técnico.
3+
// Distributed under the MIT license that can be found in the LICENSE file.
4+
5+
use Doctrine\ORM\Mapping\Column;
6+
use Doctrine\ORM\Mapping\Entity;
7+
use Doctrine\ORM\Mapping\GeneratedValue;
8+
use Doctrine\ORM\Mapping\Id;
9+
use Doctrine\ORM\Mapping\JoinColumn;
10+
use Doctrine\ORM\Mapping\ManyToOne;
11+
12+
/** @Entity */
13+
class PatchComment
14+
{
15+
/** @Id @Column @GeneratedValue */
16+
public int $id;
17+
18+
/** @ManyToOne
19+
* @JoinColumn(nullable=false)
20+
*/
21+
public Patch $patch;
22+
23+
/** @Column(length=4096) */
24+
public string $text;
25+
26+
/** @ManyToOne */
27+
public ?User $user;
28+
29+
/** @Column */
30+
public DateTimeImmutable $time;
31+
32+
public function __construct(Patch $patch, string $text, ?User $user = null) {
33+
$this->patch = $patch;
34+
$this->text = $text;
35+
$this->user = $user;
36+
$this->time = new DateTimeImmutable();
37+
}
38+
}

‎github.php

+11-1
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,15 @@ public function removePlugin(string $fqcn):void {
5353
}
5454
}
5555

56+
public function addHeaderValue(string $header, string $headerValue): void {
57+
$this->plugins[] = new HeaderSetPlugin([$header => $headerValue]);
58+
$this->client = null;
59+
}
60+
61+
public function clearHeaders(): void {
62+
$this->removePlugin(Plugin\HeaderAppendPlugin::class);
63+
}
64+
5665
public function getHttpClient(): HttpMethodsClientInterface {
5766
if (!$this->client) {
5867
$stream = Psr17FactoryDiscovery::findStreamFactory();
@@ -74,7 +83,8 @@ public function getHttpClient(): HttpMethodsClientInterface {
7483
}
7584
}
7685

77-
$github_client = new \Github\Client(new MyHttpBuilder(2 * 60));
86+
$github_builder = new MyHttpBuilder(5 * 60);
87+
$github_client = new \Github\Client($github_builder);
7888
$github_client->authenticate(GH_TOKEN, null, \Github\AuthMethod::ACCESS_TOKEN);
7989

8090
$github_client_cached = new \Github\Client(new MyHttpBuilder(10*24*3600));

‎pages/editpatch.php

+77-30
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414

1515
$readonly = ['group'];
1616
if (get_user()->role == ROLE_STUDENT) {
17-
$readonly[] = 'review';
1817
$readonly[] = 'status';
1918
}
2019

@@ -26,40 +25,79 @@
2625
echo "<p>&nbsp;</p>\n";
2726
mk_box_left_begin();
2827

29-
// if the student changes description, get the patch back on the review queue
30-
if (get_user()->role == ROLE_STUDENT &&
31-
($patch->status == PATCH_REVIEWED || $patch->status == PATCH_NOTMERGED) &&
32-
isset($_POST['description']) &&
33-
$patch->description != $_POST['description']) {
34-
$patch->set_status(PATCH_WAITING_REVIEW);
35-
36-
$user = get_user();
37-
$name = $user->shortName();
38-
email_ta($patch->group, 'PIC1: patch updated',
39-
"$name ($user) requested review for an existing patch\n" .
40-
link_patch($patch));
41-
}
42-
43-
// Add approve/reject buttons to simplify the life of TAs
44-
$extra_buttons = [];
45-
if ($patch->status <= PATCH_REVIEWED &&
46-
auth_at_least(ROLE_TA)) {
47-
$extra_buttons['Approve'] = ['status', PATCH_APPROVED];
48-
$extra_buttons['Reject'] = ['status', PATCH_REVIEWED];
49-
}
50-
5128
$prev_status = $patch->status;
5229

53-
handle_form($patch, [], $readonly,
54-
['group', 'status', 'type', 'issue_url', 'description', 'review'],
55-
$extra_buttons);
30+
handle_form($patch, [], $readonly, ['group', 'status', 'type', 'issue_url'],
31+
null, false);
5632

5733
if (auth_at_least(ROLE_PROF)) {
5834
$link = dolink('rmpatch', 'Delete', ['id' => $patch->id]);
5935
echo "<p>&nbsp;</p>\n<p>", dolink('rmpatch', 'Delete', ['id' => $patch->id]),
6036
"</p>\n";
6137
}
6238

39+
$new_comment = trim($_POST['text'] ?? '');
40+
if ($new_comment) {
41+
$user = get_user();
42+
if ($user->role == ROLE_STUDENT) {
43+
if ($patch->status == PATCH_REVIEWED || $patch->status == PATCH_NOTMERGED) {
44+
$patch->set_status(PATCH_WAITING_REVIEW);
45+
}
46+
}
47+
if ($patch->status != $prev_status) {
48+
$old = Patch::get_status_options()[$prev_status];
49+
$new = Patch::get_status_options()[$patch->status];
50+
$new_comment = "Status changed: $old → $new\n\n$new_comment";
51+
}
52+
$patch->comments->add(new PatchComment($patch, $new_comment, $user));
53+
}
54+
db_flush();
55+
56+
echo "<table>\n";
57+
foreach ($patch->comments as $comment) {
58+
if ($comment->user) {
59+
$author = $comment->user->shortName() . ' (' . $comment->user->id . ')';
60+
$photo = $comment->user->getPhoto();
61+
} else {
62+
$author = 'Bot';
63+
$photo = 'https://api.dicebear.com/9.x/bottts/svg?seed=Liliana&scale=70&baseColor=00acc1&eyes=roundFrame02&mouth=smile01&texture[]&top=antenna';
64+
}
65+
$text = nl2br(htmlspecialchars($comment->text));
66+
67+
echo <<<HTML
68+
<tr>
69+
<td><b>$author</b><br>
70+
<img src="$photo" alt="Photo"><br>
71+
{$comment->time->format('d/m/Y H:i:s')}</td>
72+
<td>$text</td>
73+
</tr>
74+
HTML;
75+
}
76+
echo "</table>\n";
77+
78+
// Add approve/reject buttons to simplify the life of TAs
79+
$extra_buttons = '';
80+
if ($patch->status <= PATCH_REVIEWED && auth_at_least(ROLE_TA)) {
81+
$app = PATCH_APPROVED;
82+
$rej = PATCH_REVIEWED;
83+
$extra_buttons = <<<HTML
84+
<input type="hidden" name="status" value="{$patch->status}">
85+
<input type="submit" value="Approve" onclick="this.form.status.value='$app'">
86+
<input type="submit" value="Reject" onclick="this.form.status.value='$rej'">
87+
HTML;
88+
}
89+
90+
echo <<<HTML
91+
<p>&nbsp;</p>
92+
<form method="post">
93+
<input type="hidden" name="submit" value="1">
94+
<input type="hidden" name="id" value="{$patch->id}">
95+
<textarea name="text" rows="5" cols="80"></textarea><br>
96+
<input type="submit" value="Add comment">
97+
$extra_buttons
98+
</form>
99+
HTML;
100+
63101
mk_box_end();
64102

65103
// notify students of the patch review
@@ -80,16 +118,25 @@
80118
email_group($patch->group, $subject, <<<EOF
81119
$line
82120
83-
Description:
84-
{$patch->description}
85-
86121
Review:
87-
{$patch->review}
122+
$new_comment
88123
89124
Patch: $patchurl
90125
$pic1link
91126
EOF);
92127
}
128+
} elseif ($new_comment) {
129+
if (get_user()->role == ROLE_STUDENT) {
130+
$name = $user->shortName();
131+
email_ta($patch->group, 'PIC1: new patch comment',
132+
"$name ($user) added a new comment:\n" .
133+
"\n$new_comment\n\n" .
134+
link_patch($patch));
135+
} else {
136+
email_group($patch->group, 'PIC1: new patch comment',
137+
"$new_comment\n\n" .
138+
link_patch($patch));
139+
}
93140
}
94141

95142
$authors = [];

‎pages/patches.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@
8787
'+' => $patch->lines_added,
8888
'-' => $patch->lines_deleted,
8989
'Files' => $patch->files_modified,
90-
'Submitter' => htmlspecialchars($patch->submitter->shortName()),
90+
'Submitter' => htmlspecialchars($patch->getSubmitter()->shortName()),
9191
'Authors' => implode(', ', $authors),
9292
];
9393
}

‎pages/rmpatch.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
db_delete($patch);
1818
echo "<p>Patch deleted</p>";
1919
} else {
20-
$name = $patch->submitter->shortName();
20+
$name = $patch->getSubmitter()->shortName();
2121
$link = dolink('rmpatch', "Yes, I'm sure",
2222
['id' => (int)$_GET['id'], 'sure' => 1]);
2323
echo <<<HTML

‎templates.php

+6-4
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,8 @@ function dolink($page, $txt, $args = []) {
8383
}
8484

8585
function mk_box_left_begin() {
86-
echo '<div style="display: inline-block"><div style="float: left">';
86+
echo '<div style="display: inline-block">',
87+
'<div style="float: left; max-width: 900px">';
8788
}
8889

8990
function mk_box_right_begin() {
@@ -130,7 +131,7 @@ function print_table($table) {
130131
}
131132

132133
function handle_form(&$obj, $hide_fields, $readonly, $only_fields = null,
133-
$extra_buttons = null) {
134+
$extra_buttons = null, $flush_db = true) {
134135
$class = new ReflectionClass($obj);
135136
$docReader = new AnnotationReader();
136137

@@ -169,7 +170,8 @@ function handle_form(&$obj, $hide_fields, $readonly, $only_fields = null,
169170
}
170171
echo "</ul></span><p>&nbsp;</p>\n";
171172
} else {
172-
db_flush();
173+
if ($flush_db)
174+
db_flush();
173175
echo '<p style="color: green">Database updated!</p>';
174176
}
175177
}
@@ -286,7 +288,7 @@ function handle_form(&$obj, $hide_fields, $readonly, $only_fields = null,
286288
$key = $args[0];
287289
$value = $args[1];
288290
echo "<input type=\"submit\" value=\"$name\"",
289-
" onclick=\"document.getElementById('$key').value = '$value'\">\n";
291+
" onclick=\"this.form.$key.value='$value'\">\n";
290292
}
291293
echo "</p>\n";
292294
}

0 commit comments

Comments
 (0)