Makes message dismissible #39

andizer · 2017-07-24T13:33:00Z

Makes the message dismissible.

Fixes #38

moorscode

⛑

moorscode · 2017-07-24T14:35:12Z

src/Whip_MessageDismisser.php

+			return $versionToConvert;
+		}
+
+		return implode( '.', explode( '.', $versionToConvert, -1 ) );


It would be safer to specify the actual number of elements needed.
This would make the method a little bit more complex, but the substr_count can be removed. Which is possibly more performance intensive than just exploding and imploding.

$parts = explode( '.', $versionToConvert, 3 ); return implode( '.', array_slice( $parts, 0, 2 ) );

moorscode · 2017-07-24T14:35:48Z

src/Whip_MessageDismisser.php

+	 * @return bool
+	 */
+	public function isDismissible() {
+		return version_compare( $this->currentVersion, $this->storage->get(), '>' );


This does not reflect the comment of the method, please swap the versions and do a lower then comparison.

moorscode · 2017-07-24T14:37:07Z

src/Whip_WPDismissOption.php

+	 * @param string $dismissedVersion The value to save.
+	 */
+	public function set( $dismissedVersion ) {
+		update_option( 'whip_dismissed', $dismissedVersion );


The option name is not very descriptive of the value it will contain.
Please consider renaming this to something like: whip_dismissed_for_wp_version

moorscode · 2017-07-24T14:37:53Z

src/Whip_WPDismissOption.php

+		return $dismissedOption;
+	}
+
+}


Missing new-line

moorscode · 2017-07-24T14:38:14Z

src/Whip_WPMessageDismissListener.php

+	 */
+	public function listen() {
+		$action = filter_input( INPUT_GET, 'action' );
+		$nonce  = filter_input(INPUT_GET, 'nonce' );


Missing space before INPUT_GET

moorscode · 2017-07-24T14:50:22Z

tests/MessageDismisserTest.php

+
+		$this->assertFalse( $dismisser->isDismissible() );
+	}
+}


Missing new-line

moorscode · 2017-07-24T14:50:33Z

tests/MessageDismisserTest.php

+
+class MessageDismisserTest extends PHPUnit_Framework_TestCase {
+
+	public function testDismiss() {


Missing documentation (@covers) - sorry user for pinging you..

moorscode · 2017-07-24T14:51:00Z

tests/MessageDismisserTest.php

+		$this->assertEquals( '4.8' , $storage->get() );
+	}
+
+	public function testIsDismissed() {


moorscode · 2017-07-24T14:51:05Z

tests/MessageDismisserTest.php

+		$this->assertTrue( $dismisser->isDismissible() );
+	}
+
+	public function testIsDismissibleNotNeeded() {


moorscode · 2017-07-24T14:52:14Z

src/Whip_MessageDismisser.php

+/**
+ * A class to dismiss messages
+ */
+final class Whip_MessageDismisser {


This cannot be a final class without an abstract class or interface implementation.

moorscode

Some small details.

moorscode · 2017-07-25T13:49:55Z

src/Whip_MessageDismisser.php

+	 *
+	 * @return string The major version number.
+	 */
+	private function toMajorVersion( $versionToConvert ) {


The access should be protected for non-final classes.

moorscode · 2017-07-25T13:50:10Z

src/Whip_MessageDismisser.php

+	private $storage;
+
+	/** @var int */
+	private $currentVersion;


The access should be protected for non-final classes

moorscode · 2017-07-25T13:50:13Z

src/Whip_MessageDismisser.php

+class Whip_MessageDismisser {
+
+	/** @var Whip_DismissStorage */
+	private $storage;


The access should be protected for non-final classes

moorscode · 2017-07-25T13:51:27Z

src/Whip_WPMessageDismissListener.php

+		);
+	}
+
+}


Missing new-line

moorscode · 2017-07-25T13:52:18Z

src/Whip_WPMessageDismissListener.php

+	 */
+	public static function get_dismissurl() {
+		return sprintf(
+			admin_url( 'index.php?action=whip_dismiss&nonce=%1$s' ),


To avoid typos, consider moving the action value to a class variable.

moorscode · 2017-07-25T13:52:39Z

src/Whip_WPMessageDismissListener.php

+	public static function get_dismissurl() {
+		return sprintf(
+			admin_url( 'index.php?action=whip_dismiss&nonce=%1$s' ),
+			wp_create_nonce( 'whip_dismiss' )


The avoid typos, consider moving the nonce value to a class variable.

schlessera

Mostly nitpicking, apart from the place where the dismiss listener is being registered. It is currently being registered regardless of whether a notice is present. It should ideally be registered within the code that registers the presenter hooks, through dependency injection.

schlessera · 2017-07-25T13:55:58Z

src/Whip_MessageDismisser.php

+	/**
+	 * Whip_MessageDismisser constructor.
+	 *
+	 * @param int                 $currentVersion The current version of the installation.


This seems to accept a string, not an int.

schlessera · 2017-07-25T14:00:23Z

src/Whip_WPDismissOption.php

+	 *
+	 * @param string $dismissedVersion The value to save.
+	 *
+	 * @return bool True when successfull.


Typo: successful

schlessera · 2017-07-25T14:11:44Z

src/Whip_WPMessageDismissListener.php

+		$action = filter_input( INPUT_GET, 'action' );
+		$nonce  = filter_input( INPUT_GET, 'nonce' );
+
+		if ( $action === 'whip_dismiss' && wp_verify_nonce( $nonce, 'whip_dismiss' ) ) {


I'd put the action into a class constant.

schlessera · 2017-07-25T14:11:59Z

src/Whip_WPMessageDismissListener.php

+	 */
+	public static function get_dismissurl() {
+		return sprintf(
+			admin_url( 'index.php?action=whip_dismiss&nonce=%1$s' ),


Action into class constant here as well.

schlessera · 2017-07-25T14:12:22Z

src/Whip_WPMessageDismissListener.php

+		);
+	}
+
+}


Missing new line.

schlessera · 2017-07-25T14:21:10Z

src/Whip_MessageDismisser.php

+
+
+/**
+ * A class to dismiss messages


Code style: Missing period

schlessera · 2017-07-25T14:21:36Z

src/interfaces/Whip_DismissStorage.php

+<?php
+
+/**
+ * Interface Whip_DismissStorage


Code style: Missing period

schlessera · 2017-07-25T14:21:47Z

src/interfaces/Whip_Listener.php

+<?php
+
+/**
+ * Interface Whip_Listener


Code style: Missing period

schlessera · 2017-07-25T14:24:56Z

tests/MessageDismisserTest.php

+			array( '4.7', '4.8', true ),
+			array( '4.7', '4.8.1', true ),
+			array( '4.7.1', '4.8.1', true ),
+		);


Also test with intermediary versions, like 4.9-alpha or similar, as people might be running beta or test versions (or -gulp- use custom-patched versions of Core).

schlessera · 2017-07-25T14:25:05Z

tests/MessageDismisserTest.php

+		);
+	}
+
+}


Missing new line.

boblinthorst · 2017-07-26T08:45:44Z

src/Whip_MessageDismisser.php

+	}
+
+	/**
+	 * Checks if the dismissed version is lower then the installation version.


lower than*

boblinthorst · 2017-07-26T08:46:10Z

src/Whip_MessageDismisser.php

+	/**
+	 * Checks if the dismissed version is lower then the installation version.
+	 *
+	 * @return bool True when saved version is bigger then the current version.


bigger than*

schlessera · 2017-07-26T09:41:55Z

src/presenters/Whip_WPMessagePresenter.php

 	 */
-	public function register_hooks() {
+	public function register_hooks( Whip_MessageDismisser $dismisser ) {


The $dismisser should be injected through the constructor as a 2nd argument and stored as a property.

schlessera · 2017-07-26T09:42:35Z

src/presenters/Whip_WPMessagePresenter.php

 		if ( null === $whip_admin_notices_added || ! $whip_admin_notices_added ) {
 			add_action( 'admin_notices', array( $this, 'renderMessage' ) );
 			$whip_admin_notices_added = true;
 		}
+
+		$dismissListener = new Whip_WPMessageDismissListener( $dismisser );


The listener is only needed if the message is actually being rendered, so it should only be created/registered in the renderMessage() method.

schlessera · 2017-07-26T09:43:07Z

src/presenters/Whip_WPMessagePresenter.php

 	}

 	/**
 	 * Renders the messages present in the global to notices.
 	 */
 	public function renderMessage() {
-	    printf( '<div class="error">%s</div>', $this->kses( $this->message->body() ) );
+		/* translators: 1: is a link to dismiss url 2: closing link tag */


Here I'd start by creating the listener and registering it.

schlessera · 2017-07-26T09:43:36Z

src/presenters/Whip_WPMessagePresenter.php

+		/* translators: 1: is a link to dismiss url 2: closing link tag */
+		$dismiss_button = sprintf(
+			__( '<p>%1$sRemind me again after the next WordPress release.%2$s</p>', 'wordpress' ),
+			'<a href="' . Whip_WPMessageDismissListener::get_dismissurl() . '">',


As we have access to the actual instance of $dismissListener now, we can get rid of the static access here.

moorscode · 2017-07-28T08:48:48Z

src/facades/wordpress.php

 		$config  = include dirname( __FILE__ ) . '/../configs/default.php';
 		$checker = new Whip_RequirementsChecker( $config );

+		$dismisser = new Whip_MessageDismisser( $wp_version, new Whip_WPDismissOption() );
+
+		if ( ! $dismisser->isDismissed() ) {


This does not read logically: "If the message has not been dismissed, stop execution."
It seems that the function is returning the inverted expected value. False should be True.

Move the `isDismissed` to the presenter

Andy Meerwaldt added 28 commits July 20, 2017 13:59

Add class for filtering requirements.

a6e6b6d

Add unit tests

7b30ee0

Add object for dismissing the message.

c657a0a

Implement both classes in the wp facade

17e4545

Added a listener interface

39a04aa

Rename the class and add some functionality.

b9c407e

Adds an interface representing the storage.

e2be0ea

Add class for the wordpress dismiss option.

d1ac35c

Moved some listener functionality.

397310b

Use the right class.

3347deb

Fix the tests

c56b087

Implement the dismisser

aff4b54

Pass the right attributes

512dd7e

Add a dismiss button to the notice.

6f21b08

Fix some small bugs

9ee70a8

Replace the filter because of php compatibility

81007d9

Remove unnecessary file.

8554d0e

Remove comparison version from the url.

1673f18

Fix typo in interface name

3e1bd3e

Change the interface.

74ad764

Update the message

1462d05

Fix return value

07d258e

Updates the listener

bf68c2b

Reduces complexity

d63b3e5

Updates the test

23a6923

Change the default textdomain.

f686b7a

Only compare major versions.

d04d63f

Handle version number correctly

1256f22

moorscode suggested changes Jul 24, 2017

View reviewed changes

Process CR.

0e8afdb

moorscode suggested changes Jul 25, 2017

View reviewed changes

schlessera requested changes Jul 25, 2017

View reviewed changes

Process CR

5ec43b1

boblinthorst reviewed Jul 26, 2017

View reviewed changes

Fix grammar

9ce712d

schlessera requested changes Jul 26, 2017

View reviewed changes

Process feedback

4cc0640

moorscode approved these changes Jul 28, 2017

View reviewed changes

moorscode suggested changes Jul 28, 2017

View reviewed changes

moorscode added 2 commits July 31, 2017 09:57

Flip the functionality around in the isDismissed method

a45d142

Move the `isDismissed` to the presenter

Update tests according to functionality in method

bbe91cf

moorscode approved these changes Jul 31, 2017

View reviewed changes

moorscode added 2 commits July 31, 2017 10:43

Apply camelCase for method and argument

67eb369

Convert to CamelCase

67fcef0

moorscode merged commit 0f38cde into master Jul 31, 2017

moorscode deleted the stories/38-make-message-dismissible branch July 31, 2017 08:53

moorscode added this to the 1.1.0 milestone Aug 1, 2017


		class MessageDismisserTest extends PHPUnit_Framework_TestCase {

		public function testDismiss() {

Makes message dismissible #39

Makes message dismissible #39

Conversation

andizer commented Jul 24, 2017 • edited Loading

moorscode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorscode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlessera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andizer commented Jul 24, 2017 •

edited

Loading