Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes message dismissible #39

Merged
merged 36 commits into from
Jul 31, 2017
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
a6e6b6d
Add class for filtering requirements.
Jul 20, 2017
7b30ee0
Add unit tests
Jul 20, 2017
c657a0a
Add object for dismissing the message.
Jul 20, 2017
17e4545
Implement both classes in the wp facade
Jul 20, 2017
39a04aa
Added a listener interface
Jul 20, 2017
b9c407e
Rename the class and add some functionality.
Jul 20, 2017
e2be0ea
Adds an interface representing the storage.
Jul 20, 2017
d1ac35c
Add class for the wordpress dismiss option.
Jul 20, 2017
397310b
Moved some listener functionality.
Jul 20, 2017
3347deb
Use the right class.
Jul 20, 2017
c56b087
Fix the tests
Jul 24, 2017
aff4b54
Implement the dismisser
Jul 24, 2017
512dd7e
Pass the right attributes
Jul 24, 2017
6f21b08
Add a dismiss button to the notice.
Jul 24, 2017
9ee70a8
Fix some small bugs
Jul 24, 2017
81007d9
Replace the filter because of php compatibility
Jul 24, 2017
8554d0e
Remove unnecessary file.
Jul 24, 2017
1673f18
Remove comparison version from the url.
Jul 24, 2017
3e1bd3e
Fix typo in interface name
Jul 24, 2017
74ad764
Change the interface.
Jul 24, 2017
1462d05
Update the message
Jul 24, 2017
07d258e
Fix return value
Jul 24, 2017
bf68c2b
Updates the listener
Jul 24, 2017
d63b3e5
Reduces complexity
Jul 24, 2017
23a6923
Updates the test
Jul 24, 2017
f686b7a
Change the default textdomain.
Jul 24, 2017
d04d63f
Only compare major versions.
Jul 24, 2017
1256f22
Handle version number correctly
Jul 24, 2017
0e8afdb
Process CR.
Jul 25, 2017
5ec43b1
Process CR
Jul 26, 2017
9ce712d
Fix grammar
Jul 26, 2017
4cc0640
Process feedback
Jul 26, 2017
a45d142
Flip the functionality around in the `isDismissed` method
moorscode Jul 31, 2017
bbe91cf
Update tests according to functionality in method
moorscode Jul 31, 2017
67eb369
Apply camelCase for method and argument
moorscode Jul 31, 2017
67fcef0
Convert to CamelCase
moorscode Jul 31, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions src/Whip_MessageDismisser.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php


/**
* A class to dismiss messages.
*/
class Whip_MessageDismisser {

/** @var Whip_DismissStorage */
protected $storage;

/** @var string */
protected $currentVersion;

/**
* Whip_MessageDismisser constructor.
*
* @param string $currentVersion The current version of the installation.
* @param Whip_DismissStorage $storage The storage object handling storage of versioning.
*/
public function __construct( $currentVersion, Whip_DismissStorage $storage ) {
$this->currentVersion = $this->toMajorVersion( $currentVersion );
$this->storage = $storage;
}

/**
* Saves the version number to the storage to indicate the message as being dismissed.
*/
public function dismiss() {
$this->storage->set( $this->currentVersion );
}

/**
* Checks if the dismissed version is lower then the installation version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lower than*

*
* @return bool True when saved version is bigger then the current version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bigger than*

*/
public function isDismissed() {
return version_compare( $this->storage->get(), $this->currentVersion, '<' );
}

/**
* Converts the version number to a major version number.
*
* @param string $versionToConvert The version to convert.
*
* @return string The major version number.
*/
protected function toMajorVersion( $versionToConvert ) {
$parts = explode( '.', $versionToConvert, 3 );

return implode( '.', array_slice( $parts, 0, 2 ) );
}
}
2 changes: 1 addition & 1 deletion src/Whip_RequirementsChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Whip_RequirementsChecker {
* @param array $configuration The configuration to check.
* @param string $textdomain The text domain to use for translations.
*/
public function __construct( $configuration = array(), $textdomain = 'wordpress-seo' ) {
public function __construct( $configuration = array(), $textdomain = 'wordpress' ) {
$this->requirements = array();
$this->configuration = new Whip_Configuration( $configuration );
$this->messageMananger = new Whip_MessagesManager();
Expand Down
36 changes: 36 additions & 0 deletions src/Whip_WPDismissOption.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

/**
* Represents the WordPress option for saving the dismissed messages.
*/
class Whip_WPDismissOption implements Whip_DismissStorage {

/** @var string */
protected $option_name = 'whip_dismissed_for_wp_version';

/**
* Saves the value to the options.
*
* @param string $dismissedVersion The value to save.
*
* @return bool True when successful.
*/
public function set( $dismissedVersion ) {
return update_option( $this->option_name, $dismissedVersion );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_option() only returns true if the value was indeed changed. If the value is already set to what you want it to update to, the call is to be considered "successful", but update_option() will return false.
Probably only a theoretical problem, though, as this method should only be called if the option has not been set to the correct version befor.

}

/**
* Returns the value of the whip_dismissed option.
*
* @return string Returns the value of the option or an empty string when not set.
*/
public function get() {
$dismissedOption = get_option( $this->option_name );
if ( ! $dismissedOption ) {
return '';
}

return $dismissedOption;
}

}
51 changes: 51 additions & 0 deletions src/Whip_WPMessageDismissListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php

/**
* Listener for dismissing a message.
*/
class Whip_WPMessageDismissListener implements Whip_Listener {

const ACTION_NAME = 'whip_dismiss';

/**
* @var Whip_MessageDismisser
*/
protected $dismisser;

/**
* Sets the dismisser attribute.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing argument & description.

*
* @param Whip_MessageDismisser $dismisser The object for dismissing a message.
*/
public function __construct( Whip_MessageDismisser $dismisser ) {
$this->dismisser = $dismisser;
}

/**
* Listens to a GET request to fetch the required attributes.
*
* @return void
*/
public function listen() {
$action = filter_input( INPUT_GET, 'action' );
$nonce = filter_input( INPUT_GET, 'nonce' );

if ( $action === self::ACTION_NAME && wp_verify_nonce( $nonce, self::ACTION_NAME ) ) {
$this->dismisser->dismiss();
}
}

/**
* Creates an url for dismissing the notice.
*
* @return string The url for dismissing the message.
*/
public static function get_dismissurl() {
return sprintf(
admin_url( 'index.php?action=%1$s&nonce=%2$s' ),
self::ACTION_NAME,
wp_create_nonce( self::ACTION_NAME )
);
}

}
11 changes: 10 additions & 1 deletion src/facades/wordpress.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,18 @@ function whip_wp_check_versions( $requirements ) {
return;
}

global $wp_version;


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

$dismisser = new Whip_MessageDismisser( $wp_version, new Whip_WPDismissOption() );

if ( ! $dismisser->isDismissed() ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}

foreach ( $requirements as $component => $versionComparison ) {
$checker->addRequirement( Whip_VersionRequirement::fromCompareString( $component, $versionComparison ) );
}
Expand All @@ -26,6 +35,6 @@ function whip_wp_check_versions( $requirements ) {
}

$presenter = new Whip_WPMessagePresenter( $checker->getMostRecentMessage() );
$presenter->register_hooks();
$presenter->register_hooks( $dismisser );
}
}
24 changes: 24 additions & 0 deletions src/interfaces/Whip_DismissStorage.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

/**
* Interface Whip_DismissStorage.
*/
interface Whip_DismissStorage {

/**
* Saves the value.
*
* @param string $dismissedVersion The value to save.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a return value with type boolean, to be able to confirm save has been successful.

*
* @return bool True when successful.
*/
public function set( $dismissedVersion );

/**
* Returns the value.
*
* @return string
*/
public function get();

}
15 changes: 15 additions & 0 deletions src/interfaces/Whip_Listener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

/**
* Interface Whip_Listener.
*/
interface Whip_Listener {

/**
* Method that should implement the listen functionality.
*
* @return void
*/
public function listen();

}
17 changes: 15 additions & 2 deletions src/presenters/Whip_WPMessagePresenter.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,33 @@ public function __construct( Whip_Message $message ) {
/**
* Registers hooks to WordPress. This is a separate function so you can
* control when the hooks are registered.
*
* @param Whip_MessageDismisser $dismisser Dismisser object.
*/
public function register_hooks() {
public function register_hooks( Whip_MessageDismisser $dismisser ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

global $whip_admin_notices_added;

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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$dismissListener->listen();
}

/**
* 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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$dismiss_button = sprintf(
__( '<p>%1$sRemind me again after the next WordPress release.%2$s</p>', 'wordpress' ),
'<a href="' . Whip_WPMessageDismissListener::get_dismissurl() . '">',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'</a>'
);

printf( '<div class="error">%1$s<p>%2$s</p></div>', $this->kses( $this->message->body() ), $dismiss_button );
}

/**
Expand Down
72 changes: 72 additions & 0 deletions tests/MessageDismisserTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php

class Whip_DismissStorageMock implements Whip_DismissStorage {

/** @var string */
protected $dismissed = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing description


/**
* Saves the value.
*
* @param string $dismissedVersion The value to save.
*/
public function set( $dismissedVersion ) {
$this->dismissed = $dismissedVersion;
}

/**
* Returns the value.
*
* @return string
*/
public function get() {
return $this->dismissed;
}
}

class MessageDismisserTest extends PHPUnit_Framework_TestCase {

/**
* @covers Whip_MessageDismisser::__construct()
* @covers Whip_MessageDismisser::dismiss()
*/
public function testDismiss() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$storage = new Whip_DismissStorageMock();
$dismisser = new Whip_MessageDismisser( '4.8', $storage );

$dismisser->dismiss();

$this->assertEquals( '4.8' , $storage->get() );
}

/**
* @dataProvider versionNumbersProvider
*
* @param string $savedVersion The saved version number.
* @param string $currentVersion The current version number.
* @param bool $expected The expected value.
*
* @covers Whip_MessageDismisser::__construct()
* @covers Whip_MessageDismisser::isDismissed()
* @covers Whip_MessageDismisser::toMajorVersion()
*/
public function testIsDismissibleWithVersions( $savedVersion, $currentVersion, $expected ) {
$storage = new Whip_DismissStorageMock();
$storage->set( $savedVersion );
$dismisser = new Whip_MessageDismisser( $currentVersion, $storage );

$this->assertEquals( $expected, $dismisser->isDismissed() );
}

public function versionNumbersProvider() {
return array(
array( '4.8', '4.8', false ),
array( '4.8', '4.8.1', false ),
array( '4.7', '4.8', true ),
array( '4.7', '4.8.1', true ),
array( '4.7.1', '4.8.1', true ),
array( '4.7', '4.7-alpha', false ),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

}