Merge branch 'refactor-module-rbmark' into 'dev'

Refactor and cleanup Rbmark module + add tests

See merge request hubzilla/core!2126

5 files changed, 143 insertions, 60 deletions

diff --git a/Zotlabs/Module/Rbmark.php b/Zotlabs/Module/Rbmark.php
index 87b774495..b1aea2590 100644
--- a/Zotlabs/Module/Rbmark.php
+++ b/Zotlabs/Module/Rbmark.php
@@ -1,5 +1,5 @@
 <?php
-namespace Zotlabs\Module; /** @file */
+namespace Zotlabs\Module;
 
 require_once('include/acl_selectors.php');
 require_once('include/crypto.php');
@@ -23,11 +23,9 @@ require_once('include/bookmarks.php');
  * remote_return= absolute URL to return after posting is finished
  *
  */
-
-
 class Rbmark extends \Zotlabs\Web\Controller {
 
-	function post() {
+	public function post(): void {
 		if($_POST['submit'] !== t('Save'))
 			return;
 
@@ -36,22 +34,21 @@ class Rbmark extends \Zotlabs\Web\Controller {
 		$channel = \App::get_channel();
 
 		$t = array('url' => escape_tags($_REQUEST['url']),'term' => escape_tags($_REQUEST['title']));
+
 		bookmark_add($channel,$channel,$t,((x($_REQUEST,'private')) ? intval($_REQUEST['private']) : 0),
 			array('menu_id' => ((x($_REQUEST,'menu_id')) ? intval($_REQUEST['menu_id']) : 0),
-				'menu_name' => ((x($_REQUEST,'menu_name')) ? escape_tags($_REQUEST['menu_name']) : ''),
-				'ischat' => ((x($_REQUEST['ischat'])) ? intval($_REQUEST['ischat']) : 0)
+			'menu_name' => ((x($_REQUEST,'menu_name')) ? escape_tags($_REQUEST['menu_name']) : ''),
+			'ischat' => ((x($_REQUEST['ischat'])) ? intval($_REQUEST['ischat']) : 0)
 			));
 
 		goaway(z_root() . '/bookmarks');
-
 	}
 
 
-		function get() {
+	public function get(): string {
 
-		$o = '';
-
-		if(! local_channel()) {
+		$channel_id = local_channel();
+		if($channel_id === false) {
 
 			// The login procedure is going to bugger our $_REQUEST variables
 			// so save them in the session.
@@ -62,59 +59,45 @@ class Rbmark extends \Zotlabs\Web\Controller {
 			return login();
 		}
 
-		// If we have saved rbmark session variables, but nothing in the current $_REQUEST, recover the saved variables
+		// If we have saved rbmark session variables, but nothing in the
+		// current $_REQUEST, recover the saved variables
 
 		if((! array_key_exists('url',$_REQUEST)) && (array_key_exists('bookmark',$_SESSION))) {
 			$_REQUEST = $_SESSION['bookmark'];
 			unset($_SESSION['bookmark']);
 		}
 
-		if($_REQUEST['remote_return']) {
-			$_SESSION['remote_return'] = $_REQUEST['remote_return'];
-		}
-		if(argc() > 1 && argv(1) === 'return') {
-			if($_SESSION['remote_return'])
-				goaway($_SESSION['remote_return']);
-			goaway(z_root() . '/bookmarks');
-		}
-
-		$channel = \App::get_channel();
-
-
-		$m = menu_list($channel['channel_id'],'',MENU_BOOKMARK);
-
-		$menus = array();
-		if($m) {
-			$menus = array(0 => '');
-			foreach($m as $n) {
-				$menus[$n['menu_id']] = $n['menu_name'];
-			}
-		}
-		$menu_select = array('menu_id',t('Select a bookmark folder'),false,'',$menus);
-
-
-		$o .= replace_macros(get_markup_template('rbmark.tpl'), array(
+		$menu_select = [
+			'menu_id',
+			t('Select a bookmark folder'),
+			false,
+			'',
+			$this->get_bookmark_folders(intval($channel_id)),
+		];
 
+		return replace_macros(get_markup_template('rbmark.tpl'), array(
 			'$header' => t('Save Bookmark'),
-			'$url' => array('url',t('URL of bookmark'),escape_tags($_REQUEST['url'])),
-			'$title' => array('title',t('Description'),escape_tags($_REQUEST['title'])),
+			'$url' => array('url',t('URL of bookmark'),$_REQUEST['url']),
+			'$title' => array('title',t('Description'),$_REQUEST['title']),
 			'$ischat' => ((x($_REQUEST,'ischat')) ? intval($_REQUEST['ischat']) : 0),
 			'$private' => ((x($_REQUEST,'private')) ? intval($_REQUEST['private']) : 0),
 			'$submit' => t('Save'),
 			'$menu_name' => array('menu_name',t('Or enter new bookmark folder name'),'',''),
 			'$menus' => $menu_select
-
 		));
+	}
 
+	private function get_bookmark_folders(int $channel_id): array {
+		$menu_list = menu_list($channel_id, '', MENU_BOOKMARK);
 
+		$menus = [ 0 => '' ];
 
+		if ($menu_list !== false) {
+			foreach($menu_list as $n) {
+				$menus[$n['menu_id']] = $n['menu_name'];
+			}
+		}
 
-
-
-		return $o;
-
+		return $menus;
 	}
-
-
-
 }
diff --git a/tests/unit/Module/HelpTest.php b/tests/unit/Module/HelpTest.php
index 0f1610db5..3fb7687d3 100644
--- a/tests/unit/Module/HelpTest.php
+++ b/tests/unit/Module/HelpTest.php
@@ -176,13 +176,4 @@ class HelpTest extends \Zotlabs\Tests\Unit\Module\TestCase {
 
 		$this->assertPageContains('<h3>This is the included file.</h3>');
 	}
-
-	/**
-	 * Helper to simplify asserting contents in the rendered page.
-	 *
-	 * @param string $needle	The expected string to find.
-	 */
-	private function assertPageContains(string $needle): void {
-		$this->assertStringContainsString($needle, \App::$page['content']);
-	}
 }
diff --git a/tests/unit/Module/RbmarkTest.php b/tests/unit/Module/RbmarkTest.php
new file mode 100644
index 000000000..594e7369b
--- /dev/null
+++ b/tests/unit/Module/RbmarkTest.php
@@ -0,0 +1,80 @@
+<?php
+/**
+ * Unit/integration tests for the Rbmark module.
+ *
+ * SPDX-FileCopyrightText: 2024 Hubzilla Community
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+class RbmarkTest extends \Zotlabs\Tests\Unit\Module\TestCase {
+	public function test_unauthenticated_get_request_return_login_form(): void {
+		$lc_stub = $this->getFunctionMock('Zotlabs\Module', 'local_channel');
+		$lc_stub
+			->expects($this->once())
+			->willReturn(false);
+
+		$this->get('rbmark', ['url' => 'https://bookmarked.url']);
+
+		$this->assertPageContains('value="login" />');
+
+		// also check that the original query is saved in the session
+		$this->assertEquals('https://bookmarked.url', $_SESSION['bookmark']['url']);
+		$this->assertEquals('rbmark', $_SESSION['bookmark']['q']);
+	}
+
+	public function test_authenticated_get_request_returns_save_bookmark_form(): void {
+		$lc_stub = $this->getFunctionMock('Zotlabs\Module', 'local_channel');
+		$lc_stub
+			->expects($this->once())
+			->willReturn(42);
+
+		$this->get('rbmark', [
+			'url' => 'https://bookmarked.url',
+			'title' => 'My bookmark',
+		]);
+
+		$this->assertPageContains('<form action="rbmark" method="post"');
+		$this->assertPageContains('URL of bookmark');
+		$this->assertPageContains('value="https://bookmarked.url"');
+		$this->assertPageContains('value="My bookmark"');
+	}
+
+	public function test_that_params_are_escaped_in_save_bookmark_form(): void {
+		$lc_stub = $this->getFunctionMock('Zotlabs\Module', 'local_channel');
+		$lc_stub
+			->expects($this->once())
+			->willReturn(42);
+
+		$this->get('rbmark', [
+			'url' => 'https://bookmarked.url" onload="alert(/boom/)',
+			'title' => 'My bookmark"><script alert(/boom/);</script>',
+		]);
+
+		$this->assertPageContains('value="https://bookmarked.url&quot; onload=&quot;alert(/boom/)');
+		$this->assertPageContains('value="My bookmark&quot;&gt;&lt;script alert(/boom/);&lt;/script&gt;');
+	}
+
+	public function test_that_existing_bookmark_folders_are_listed(): void {
+		$lc_stub = $this->getFunctionMock('Zotlabs\Module', 'local_channel');
+		$lc_stub
+			->expects($this->once())
+			->willReturn(42);
+
+		$menu_id = menu_create([
+			'menu_name' => 'My bookmarks',
+			'menu_desc' => 'A collection of my bookmarks',
+			'menu_flags' => MENU_BOOKMARK,
+			'menu_channel_id' => 42,
+		]);
+
+		$this->get('rbmark', [
+			'url' => 'https://bookmarked.url',
+			'title' => 'My bookmark',
+		]);
+
+		$this->assertPageContains(
+			"<option value=\"{$menu_id}\" >My bookmarks</option>"
+		);
+	}
+}
diff --git a/tests/unit/Module/TestCase.php b/tests/unit/Module/TestCase.php
index aa09e0596..5ad213e81 100644
--- a/tests/unit/Module/TestCase.php
+++ b/tests/unit/Module/TestCase.php
@@ -12,10 +12,19 @@ class TestCase extends \Zotlabs\Tests\Unit\UnitTestCase {
 	 *
 	 * @param string $uri	The URI to request. Typically this will be the module
 	 *						name, followed by any req args separated by slashes.
+	 *
+	 * @param array $query	Assciative array of query args, with the parameters
+	 *						as keys.
 	 */
-	protected function get(string $uri): void {
+	protected function get(string $uri, array $query = []): void {
 		$_GET['q'] = $uri;
+
+		if (!empty($query)) {
+			$_GET = array_merge($_GET, $query);
+		}
+
 		$_SERVER['REQUEST_METHOD'] = 'GET';
+		$_REQUEST = $_GET;
 
 		\App::init();
 		\App::$page['content'] = '';
@@ -25,6 +34,15 @@ class TestCase extends \Zotlabs\Tests\Unit\UnitTestCase {
 	}
 
 	/**
+	 * Helper to simplify asserting contents in the rendered page.
+	 *
+	 * @param string $needle	The expected string to find.
+	 */
+	protected function assertPageContains(string $needle): void {
+		$this->assertStringContainsString($needle, \App::$page['content']);
+	}
+
+	/**
 	 * Stub out the `killme` function.
 	 *
 	 * Usefule for modules that call this function directly.
diff --git a/view/tpl/field_input.tpl b/view/tpl/field_input.tpl
index 2f9f83ac5..1b548221f 100644
--- a/view/tpl/field_input.tpl
+++ b/view/tpl/field_input.tpl
@@ -1,7 +1,18 @@
 	<div id="id_{{$field.0}}_wrapper" class="mb-3">
-		<label for="id_{{$field.0}}" id="label_{{$field.0}}">{{$field.1}}{{if $field.4}}<sup class="required zuiqmid"> {{$field.4}}</sup>{{/if}}</label>
-		<input class="form-control" name="{{$field.0}}" id="id_{{$field.0}}" type="text" value="{{$field.2}}"{{if $field.5}} {{$field.5}}{{/if}}>
-		<small id="help_{{$field.0}}" class="form-text text-muted">{{$field.3}}</small>
+		<label for="id_{{$field.0}}" id="label_{{$field.0}}">
+			{{$field.1}}{{if $field.4}}<sup class="required zuiqmid"> {{$field.4}}</sup>{{/if}}
+		</label>
+		<input
+			class="form-control"
+			name="{{$field.0}}"
+			id="id_{{$field.0}}"
+			type="text"
+			value="{{$field.2|escape:'html':'UTF-8':FALSE}}"
+			{{if $field.5}}{{$field.5}}{{/if}}
+			>
+		<small id="help_{{$field.0}}" class="form-text text-muted">
+			{{$field.3}}
+		</small>
 	</div>
 {{*
 	COMMENTS for this template: