From 0d20d4f51aac1e618b5a446f0303d26b636f6bac Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 20 Oct 2026 06:49:16 -0500 Subject: [PATCH] Fix raw language keys and validation findings The claim page's back link used RETURN_TO_TOPIC, which phpBB does not define, and MCP_TOPICCLAIM_CLAIMS, which only loads inside the MCP, so both showed as raw keys. Use the extension's own strings instead. From a validation pass: - Ask for confirmation before releasing or taking over a claim - Simplify the claim page's action switch and add a default case - Send one email template per reason, so the whole email is in the recipient's language - Clamp the MCP list's start to the last page - Name the magic numbers (redirect delay, seconds per day, item ID mask) - Remove unused code: the notify_icon property, three unused template variables and an unused parameter with a wrong docblock Co-Authored-By: Claude Opus 5.5 --- claims.php | 5 +- controller/manage.php | 105 +++++++++++++-------- language/en/email/topicclaim_notify.txt | 11 --- language/en/email/topicclaim_reassign.txt | 11 +++ language/en/email/topicclaim_release.txt | 11 +++ language/en/email/topicclaim_takeover.txt | 11 +++ language/en/email/topicclaim_transfer.txt | 11 +++ language/en/topicclaim.php | 9 +- mcp/main_module.php | 13 ++- notification/claim.php | 23 +++-- styles/all/template/topicclaim_manage.html | 2 +- 11 files changed, 141 insertions(+), 71 deletions(-) delete mode 100644 language/en/email/topicclaim_notify.txt create mode 100644 language/en/email/topicclaim_reassign.txt create mode 100644 language/en/email/topicclaim_release.txt create mode 100644 language/en/email/topicclaim_takeover.txt create mode 100644 language/en/email/topicclaim_transfer.txt diff --git a/claims.php b/claims.php index 3b81edf..127f8aa 100644 --- a/claims.php +++ b/claims.php @@ -27,6 +27,9 @@ class claims /** Longest note, in characters; the column holds 255 */ const NOTE_MAX_LENGTH = 255; + /** Seconds in a day, for the release-after-days setting */ + const SECONDS_PER_DAY = 86400; + /** Notification reasons */ const REASON_TRANSFER = 'transfer'; const REASON_TAKEOVER = 'takeover'; @@ -497,7 +500,7 @@ public function find_expired($days) { $sql = 'SELECT topic_id FROM ' . $this->claims_table . ' - WHERE claim_time < ' . (time() - (int) $days * 86400); + WHERE claim_time < ' . (time() - (int) $days * self::SECONDS_PER_DAY); return $this->fetch_topic_ids($sql); } diff --git a/controller/manage.php b/controller/manage.php index 2e8ad0b..63c748c 100644 --- a/controller/manage.php +++ b/controller/manage.php @@ -32,6 +32,18 @@ class manage /** Form key name */ const FORM_KEY = 'phpbbmodders_topicclaim'; + /** Seconds the result message shows before going back */ + const REDIRECT_DELAY = 3; + + /** + * Buttons that ask for confirmation first, with the title key confirm_box() uses. + * It adds _CONFIRM for the question: TOPICCLAIM_RELEASE_CONFIRM, TOPICCLAIM_TAKEOVER_CONFIRM. + */ + const CONFIRM_ACTIONS = [ + 'release' => 'TOPICCLAIM_RELEASE', + 'takeover' => 'TOPICCLAIM_TAKEOVER', + ]; + /** @var auth */ protected $auth; @@ -118,22 +130,42 @@ public function handle($topic_id) $u_topic = append_sid("{$this->root_path}viewtopic.{$this->php_ext}", 't=' . $topic['topic_id']); $u_page = $this->helper->route('phpbbmodders_topicclaim_manage', ['topic_id' => $topic['topic_id']]); $u_return = $this->get_return_url($u_topic); + $u_action = $u_page . ($u_return !== $u_topic ? (strpos($u_page, '?') === false ? '?' : '&') . 'return=mcp' : ''); add_form_key(self::FORM_KEY); $action = $this->get_action(); if ($action !== '') { - if (!check_form_key(self::FORM_KEY)) + // "No" on the confirmation page: back to the claim page + if ($this->request->is_set_post('cancel')) + { + redirect(str_replace('&', '&', $u_action)); + } + + $needs_confirm = isset(self::CONFIRM_ACTIONS[$action]); + $confirmed = $needs_confirm && confirm_box(true); + + // confirm_box() checks its own key; every other submit has the form token + if (!$confirmed && !check_form_key(self::FORM_KEY)) { - trigger_error($this->language->lang('FORM_INVALID') . $this->return_links($u_page, $u_topic, $u_return), E_USER_WARNING); + trigger_error($this->language->lang('FORM_INVALID') . $this->return_links($u_topic, $u_return), E_USER_WARNING); + } + + if ($needs_confirm && !$confirmed) + { + confirm_box(false, self::CONFIRM_ACTIONS[$action], build_hidden_fields([ + 'topicclaim_action' => $action, + // Already HTML-escaped by request_interface + 'topicclaim_note' => $this->request->variable('topicclaim_note', '', true), + ]), 'confirm_body.html', $u_action); } $message = $this->run_action($action, $topic, $claim); - $this->finish($message, $u_page, $u_topic, $u_return); + $this->finish($message, $u_topic, $u_return); } - $this->assign_page($topic, $claim, $u_page, $u_topic, $u_return); + $this->assign_page($topic, $claim, $u_action, $u_topic, $u_return); return $this->helper->render('@phpbbmodders_topicclaim/topicclaim_manage.html', $this->language->lang('TOPICCLAIM_PAGE_TITLE')); } @@ -189,71 +221,70 @@ protected function run_action($action, array $topic, $claim) return 'TOPICCLAIM_NOT_CLAIMED'; } + // Take over is for another moderator's claim; the rest for the holder or an administrator + $allowed = ($action === 'takeover') ? (!$this->claims->is_own($claim) && $this->claims->can_override()) : $this->claims->can_manage($claim, $topic['forum_id']); + if (!$allowed) + { + return 'TOPICCLAIM_NOT_ALLOWED'; + } + switch ($action) { case 'note': - if (!$this->claims->can_manage($claim, $topic['forum_id'])) - { - break; - } $this->claims->set_note($topic, $note); - return 'TOPICCLAIM_NOTE_SAVED'; + $message = 'TOPICCLAIM_NOTE_SAVED'; + break; case 'takeover': - if ($this->claims->is_own($claim) || !$this->claims->can_override()) - { - break; - } $this->claims->take_over($topic, $claim, $note); - return 'TOPICCLAIM_TAKEN_OVER'; + $message = 'TOPICCLAIM_TAKEN_OVER'; + break; case 'transfer': - if (!$this->claims->can_manage($claim, $topic['forum_id'])) - { - break; - } - $user_id = $this->request->variable('topicclaim_user', 0); $moderators = $this->claims->get_moderators($topic['forum_id']); if (!isset($moderators[$user_id]) || $user_id === $claim['user_id']) { - return 'TOPICCLAIM_TRANSFER_INVALID'; + $message = 'TOPICCLAIM_TRANSFER_INVALID'; + break; } $this->claims->transfer($topic, $claim, $user_id, $moderators[$user_id]); - return 'TOPICCLAIM_TRANSFERRED'; + $message = 'TOPICCLAIM_TRANSFERRED'; + break; case 'release': - if (!$this->claims->can_manage($claim, $topic['forum_id'])) - { - break; - } $this->claims->release($topic, $claim); - return 'TOPICCLAIM_RELEASED'; + $message = 'TOPICCLAIM_RELEASED'; + break; + + default: + // get_action() only returns the buttons above + $message = 'TOPICCLAIM_NOT_ALLOWED'; + break; } - return 'TOPICCLAIM_NOT_ALLOWED'; + return $message; } /** * Show the result of a button and go back to where the user came from * * @param string $message Language key - * @param string $u_page * @param string $u_topic * @param string $u_return * @return void */ - protected function finish($message, $u_page, $u_topic, $u_return) + protected function finish($message, $u_topic, $u_return) { $success = in_array($message, ['TOPICCLAIM_CLAIMED', 'TOPICCLAIM_NOTE_SAVED', 'TOPICCLAIM_TAKEN_OVER', 'TOPICCLAIM_TRANSFERRED', 'TOPICCLAIM_RELEASED'], true); if ($success) { - meta_refresh(3, $u_return); + meta_refresh(self::REDIRECT_DELAY, $u_return); } - trigger_error($this->language->lang($message) . $this->return_links($u_page, $u_topic, $u_return), $success ? E_USER_NOTICE : E_USER_WARNING); + trigger_error($this->language->lang($message) . $this->return_links($u_topic, $u_return), $success ? E_USER_NOTICE : E_USER_WARNING); } /** @@ -261,12 +292,12 @@ protected function finish($message, $u_page, $u_topic, $u_return) * * @param array $topic * @param array|null $claim - * @param string $u_page + * @param string $u_action Where the page's form posts to * @param string $u_topic * @param string $u_return * @return void */ - protected function assign_page(array $topic, $claim, $u_page, $u_topic, $u_return) + protected function assign_page(array $topic, $claim, $u_action, $u_topic, $u_return) { $can_manage = $claim !== null && $this->claims->can_manage($claim, $topic['forum_id']); $moderators = []; @@ -290,12 +321,11 @@ protected function assign_page(array $topic, $claim, $u_page, $u_topic, $u_retur 'TOPICCLAIM_FORUM_NAME' => $topic['forum_name'], 'U_TOPICCLAIM_TOPIC' => $u_topic, 'U_TOPICCLAIM_FORUM' => append_sid("{$this->root_path}viewforum.{$this->php_ext}", 'f=' . $topic['forum_id']), - 'U_TOPICCLAIM_ACTION' => $u_page . ($u_return !== $u_topic ? (strpos($u_page, '?') === false ? '?' : '&') . 'return=mcp' : ''), + 'U_TOPICCLAIM_ACTION' => $u_action, 'U_TOPICCLAIM_RETURN' => $u_return, 'S_TOPICCLAIM_RETURN_MCP' => $u_return !== $u_topic, 'S_TOPICCLAIM_CLAIMED' => $claim !== null, - 'S_TOPICCLAIM_OWN' => $claim !== null && $this->claims->is_own($claim), 'S_TOPICCLAIM_CAN_MANAGE' => $can_manage, 'S_TOPICCLAIM_CAN_TAKEOVER' => $claim !== null && !$this->claims->is_own($claim) && $this->claims->can_override(), 'S_TOPICCLAIM_CAN_TRANSFER' => $can_manage && !empty($moderators), @@ -344,14 +374,13 @@ protected function get_return_url($u_topic) } /** - * Links back to the page, the topic and, when it is different, the MCP list + * Links back to the topic and, when the page was opened from it, the MCP list * - * @param string $u_page * @param string $u_topic * @param string $u_return * @return string HTML */ - protected function return_links($u_page, $u_topic, $u_return) + protected function return_links($u_topic, $u_return) { $links = '

' . $this->language->lang('RETURN_TOPIC', '', ''); diff --git a/language/en/email/topicclaim_notify.txt b/language/en/email/topicclaim_notify.txt deleted file mode 100644 index f84734e..0000000 --- a/language/en/email/topicclaim_notify.txt +++ /dev/null @@ -1,11 +0,0 @@ -Subject: "{SITENAME}" — Topic claim changed - -Hello {USERNAME}, - -{AUTHOR_NAME} {CLAIM_CHANGE} the topic "{TOPIC_TITLE}". - -View the topic: {U_TOPIC} - -You are receiving this because you claimed the topic, or a claim on it was transferred to you. Change how you are notified under Notification settings in your control panel. - -{EMAIL_SIG} diff --git a/language/en/email/topicclaim_reassign.txt b/language/en/email/topicclaim_reassign.txt new file mode 100644 index 0000000..e56248f --- /dev/null +++ b/language/en/email/topicclaim_reassign.txt @@ -0,0 +1,11 @@ +Subject: "{SITENAME}" — Your topic claim was given to another moderator + +Hello {USERNAME}, + +{AUTHOR_NAME} gave your claim on the topic "{TOPIC_TITLE}" to another moderator. + +View the topic: {U_TOPIC} + +You are receiving this because you held this claim. Change how you are notified under Notification settings in your control panel. + +{EMAIL_SIG} diff --git a/language/en/email/topicclaim_release.txt b/language/en/email/topicclaim_release.txt new file mode 100644 index 0000000..a2f61ad --- /dev/null +++ b/language/en/email/topicclaim_release.txt @@ -0,0 +1,11 @@ +Subject: "{SITENAME}" — Your topic claim was released + +Hello {USERNAME}, + +{AUTHOR_NAME} released your claim on the topic "{TOPIC_TITLE}". + +View the topic: {U_TOPIC} + +You are receiving this because you held this claim. Change how you are notified under Notification settings in your control panel. + +{EMAIL_SIG} diff --git a/language/en/email/topicclaim_takeover.txt b/language/en/email/topicclaim_takeover.txt new file mode 100644 index 0000000..4ad6a8b --- /dev/null +++ b/language/en/email/topicclaim_takeover.txt @@ -0,0 +1,11 @@ +Subject: "{SITENAME}" — Your topic claim was taken over + +Hello {USERNAME}, + +{AUTHOR_NAME} took over your claim on the topic "{TOPIC_TITLE}". + +View the topic: {U_TOPIC} + +You are receiving this because you held this claim. Change how you are notified under Notification settings in your control panel. + +{EMAIL_SIG} diff --git a/language/en/email/topicclaim_transfer.txt b/language/en/email/topicclaim_transfer.txt new file mode 100644 index 0000000..fd945fc --- /dev/null +++ b/language/en/email/topicclaim_transfer.txt @@ -0,0 +1,11 @@ +Subject: "{SITENAME}" — Topic claim transferred to you + +Hello {USERNAME}, + +{AUTHOR_NAME} transferred the claim on the topic "{TOPIC_TITLE}" to you. + +View the topic: {U_TOPIC} + +You are receiving this because a claim was transferred to you. Change how you are notified under Notification settings in your control panel. + +{EMAIL_SIG} diff --git a/language/en/topicclaim.php b/language/en/topicclaim.php index 7c7df55..765cac0 100644 --- a/language/en/topicclaim.php +++ b/language/en/topicclaim.php @@ -51,6 +51,8 @@ 'TOPICCLAIM_TRANSFER_EXPLAIN' => 'Moderators who can claim topics in this forum. They are told.', 'TOPICCLAIM_RELEASE' => 'Release claim', 'TOPICCLAIM_RELEASE_EXPLAIN' => 'Removes the claim, so another moderator can claim the topic.', + 'TOPICCLAIM_RELEASE_CONFIRM' => 'Are you sure you want to release the claim on this topic?', + 'TOPICCLAIM_TAKEOVER_CONFIRM' => 'Are you sure you want to take over this claim? The moderator who holds it is told.', 'TOPICCLAIM_VIEW_ONLY' => 'Only the moderator who holds the claim, or an administrator, can change it.', 'TOPICCLAIM_LOCKED_NOTICE' => 'This topic is locked. The board releases claims on locked topics, so it cannot be claimed.', 'TOPICCLAIM_LOCKED_RELEASE' => 'The board releases claims on locked topics. Locking this topic releases its claim.', @@ -59,6 +61,8 @@ 2 => 'The board releases claims after %d days.', ], 'TOPICCLAIM_RETURN_LIST' => '%sReturn to the claimed topics%s', + 'TOPICCLAIM_RETURN_TOPIC' => 'Return to the topic', + 'TOPICCLAIM_RETURN_CLAIMS' => 'Return to the claimed topics', // Results 'TOPICCLAIM_CLAIMED' => 'You claimed the topic.', @@ -91,11 +95,6 @@ 'TOPICCLAIM_NOTIFICATION_TAKEOVER' => 'Topic claim taken over by %1$s:', 'TOPICCLAIM_NOTIFICATION_RELEASE' => 'Topic claim released by %1$s:', 'TOPICCLAIM_NOTIFICATION_REASSIGN' => 'Topic claim given to another moderator by %1$s:', - 'TOPICCLAIM_EMAIL_TRANSFER' => 'transferred to you the claim on', - 'TOPICCLAIM_EMAIL_TAKEOVER' => 'took over your claim on', - 'TOPICCLAIM_EMAIL_RELEASE' => 'released your claim on', - 'TOPICCLAIM_EMAIL_REASSIGN' => 'gave another moderator your claim on', - // Logs 'LOG_TOPICCLAIM_CLAIM' => 'Claimed topic
» %s', 'LOG_TOPICCLAIM_TAKEOVER' => 'Took over the claim of %2$s on topic
» %1$s', diff --git a/mcp/main_module.php b/mcp/main_module.php index b922d37..c76e2d0 100644 --- a/mcp/main_module.php +++ b/mcp/main_module.php @@ -51,7 +51,16 @@ public function main($id, $mode) array_keys($auth->acl_getf('f_read', true)) )); - list($rows, $total) = $claims->get_list($forum_ids, $show === 'mine' ? (int) $user->data['user_id'] : 0, $start, self::PER_PAGE); + $user_id = ($show === 'mine') ? (int) $user->data['user_id'] : 0; + list($rows, $total) = $claims->get_list($forum_ids, $user_id, $start, self::PER_PAGE); + + // A page past the end (claims released since) goes to the last page + $valid_start = $pagination->validate_start($start, self::PER_PAGE, $total); + if ($valid_start !== $start) + { + $start = $valid_start; + list($rows, $total) = $claims->get_list($forum_ids, $user_id, $start, self::PER_PAGE); + } foreach ($rows as $row) { @@ -66,7 +75,6 @@ public function main($id, $mode) 'U_TOPIC' => append_sid("{$phpbb_root_path}viewtopic.$phpEx", 't=' . $row['topic_id']), 'U_FORUM' => append_sid("{$phpbb_root_path}viewforum.$phpEx", 'f=' . $row['forum_id']), 'U_MANAGE' => $u_manage . (strpos($u_manage, '?') === false ? '?' : '&') . 'return=mcp', - 'S_OWN' => $claims->is_own($row), 'S_CAN_MANAGE' => $claims->can_manage($row, $row['forum_id']), 'S_LOCKED' => $row['topic_status'] === ITEM_LOCKED, ]); @@ -82,7 +90,6 @@ public function main($id, $mode) 'U_TOPICCLAIM_MINE' => $this->u_action . '&show=mine', 'S_TOPICCLAIM_EXPIRE' => (int) $config['topicclaim_expire_days'] > 0, 'TOPICCLAIM_EXPIRE' => $language->lang('TOPICCLAIM_EXPIRE_NOTICE', (int) $config['topicclaim_expire_days']), - 'S_TOPICCLAIM_RELEASE_LOCKED' => !empty($config['topicclaim_release_locked']), ]); } } diff --git a/notification/claim.php b/notification/claim.php index c6bbccc..6235adb 100644 --- a/notification/claim.php +++ b/notification/claim.php @@ -27,15 +27,17 @@ class claim extends \phpbb\notification\type\base claims::REASON_REASSIGN => 'TOPICCLAIM_NOTIFICATION_REASSIGN', ]; - /** Language keys of the email wording, by reason */ - const EMAIL_CHANGES = [ - claims::REASON_TRANSFER => 'TOPICCLAIM_EMAIL_TRANSFER', - claims::REASON_TAKEOVER => 'TOPICCLAIM_EMAIL_TAKEOVER', - claims::REASON_RELEASE => 'TOPICCLAIM_EMAIL_RELEASE', - claims::REASON_REASSIGN => 'TOPICCLAIM_EMAIL_REASSIGN', + /** Email templates, by reason, so the whole email is in the recipient's language */ + const EMAIL_TEMPLATES = [ + claims::REASON_TRANSFER => '@phpbbmodders_topicclaim/topicclaim_transfer', + claims::REASON_TAKEOVER => '@phpbbmodders_topicclaim/topicclaim_takeover', + claims::REASON_RELEASE => '@phpbbmodders_topicclaim/topicclaim_release', + claims::REASON_REASSIGN => '@phpbbmodders_topicclaim/topicclaim_reassign', ]; - protected $notify_icon = 'notify_phpbb'; + /** Keeps item IDs positive signed 32-bit integers, as the notifications table stores them */ + const ITEM_ID_MASK = 0x7fffffff; + protected $permission = 'm_topicclaim'; public static $notification_option = [ @@ -81,7 +83,7 @@ public function is_available() */ public static function get_item_id($type_data) { - return (int) (crc32($type_data['topic_id'] . '|' . $type_data['actor_id'] . '|' . $type_data['recipient_id'] . '|' . $type_data['reason'] . '|' . $type_data['time']) & 0x7fffffff); + return (int) (crc32($type_data['topic_id'] . '|' . $type_data['actor_id'] . '|' . $type_data['recipient_id'] . '|' . $type_data['reason'] . '|' . $type_data['time']) & self::ITEM_ID_MASK); } public static function get_item_parent_id($type_data) @@ -171,15 +173,12 @@ public function get_redirect_url() public function get_email_template() { - return '@phpbbmodders_topicclaim/topicclaim_notify'; + return self::EMAIL_TEMPLATES[$this->get_data('reason')] ?? self::EMAIL_TEMPLATES[claims::REASON_RELEASE]; } public function get_email_template_variables() { - $change = self::EMAIL_CHANGES[$this->get_data('reason')] ?? self::EMAIL_CHANGES[claims::REASON_RELEASE]; - return [ - 'CLAIM_CHANGE' => $this->language->lang($change), 'AUTHOR_NAME' => htmlspecialchars_decode($this->user_loader->get_username($this->get_data('actor_id'), 'username'), ENT_COMPAT), 'TOPIC_TITLE' => htmlspecialchars_decode(censor_text($this->get_data('topic_title')), ENT_COMPAT), 'U_TOPIC' => generate_board_url() . '/viewtopic.' . $this->php_ext . '?t=' . (int) $this->get_data('topic_id'), diff --git a/styles/all/template/topicclaim_manage.html b/styles/all/template/topicclaim_manage.html index 9e05b57..55da819 100644 --- a/styles/all/template/topicclaim_manage.html +++ b/styles/all/template/topicclaim_manage.html @@ -99,7 +99,7 @@

{{ lang('TOPICCLAIM_RELEASE') }}

- {% if S_TOPICCLAIM_RETURN_MCP %}{{ lang('MCP_TOPICCLAIM_CLAIMS') }}{% else %}{{ lang('RETURN_TO_TOPIC') }}{% endif %} + {% if S_TOPICCLAIM_RETURN_MCP %}{{ lang('TOPICCLAIM_RETURN_CLAIMS') }}{% else %}{{ lang('TOPICCLAIM_RETURN_TOPIC') }}{% endif %}