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

Behebe "Alle" Option verfügbar für CB Manager #1306

Draft
wants to merge 42 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
736e291
Changed none to "all" option for Restrictions
hansmorb Jul 9, 2023
3376208
Merge branch 'master' into bugfix/issue-1273
hansmorb Jul 12, 2023
e27dcb9
added tests for option sanitization and getting "All" option key
hansmorb Jul 12, 2023
c9f0a9e
fixed unit tests run unnecessarily
hansmorb Jul 12, 2023
2611930
Simplify comment
datengraben Aug 20, 2023
d529725
Add restriction migration
datengraben Aug 20, 2023
de24f14
Merge branch 'master' into bugfix/issue-1273
hansmorb Sep 19, 2023
245aa0c
Revert "Add restriction migration"
hansmorb Sep 20, 2023
9fdac64
added proper migration function #1273 and re-wrote upgrade methods
hansmorb Sep 20, 2023
c001356
added restriction validation upon post-save
hansmorb Sep 25, 2023
40594fb
fixed wrong constant used in some unit tests
hansmorb Sep 25, 2023
4b7825d
fixed expected returns for meta value "all" in all places
hansmorb Sep 25, 2023
3b0852e
added unit test for getByTimerange
hansmorb Sep 24, 2023
ad3c1b6
removed stuff left-over from cherry-pick
hansmorb Sep 25, 2023
d5395b6
merge master
hansmorb Oct 2, 2023
8d0c20a
only allow administrators to set restrictions for all items / locations
hansmorb Oct 2, 2023
c928e3b
only run validation upon publication of post
hansmorb Oct 2, 2023
4ea15f5
Merge branch 'master' into bugfix/issue-1273
hansmorb Oct 19, 2023
b8c67b8
Merge branch 'master' into bugfix/issue-1273
hansmorb Oct 22, 2023
402cd6f
safe-remove unused function after merge
hansmorb Oct 22, 2023
6cc8fc8
Merge branch 'master' into bugfix/issue-1273
hansmorb Oct 22, 2023
9766209
fixed var name for test
hansmorb Oct 22, 2023
161b5f5
fixed annotation
hansmorb Oct 26, 2023
e38ed95
re-added tests
hansmorb Oct 26, 2023
4905afc
added more annotations
hansmorb Oct 26, 2023
8fc4ef5
refactored filterPosts (removed support for empty item / location field)
hansmorb Oct 26, 2023
960b8d3
re-wrote upgrade methods
hansmorb Sep 21, 2023
ee75aab
removed test for function that does not exist (yet)
hansmorb Oct 26, 2023
0de13d1
Merge branch 'refactoring/upgrade' into bugfix/issue-1273
hansmorb Oct 26, 2023
e974b3d
removed function that has been moved
hansmorb Oct 26, 2023
70c9b12
Merge branch 'refactoring/upgrade' into bugfix/issue-1273
hansmorb Oct 26, 2023
88fedba
added task that we missed
hansmorb Oct 26, 2023
f20ef13
added PHPdocs
hansmorb Oct 26, 2023
2c4fbd6
fixed syntax for new installations
hansmorb Oct 26, 2023
dbfe232
added more comments
hansmorb Oct 26, 2023
fed0699
Fix phpcs issues on new file
datengraben Oct 27, 2023
8204702
removes import and adds type annotation
datengraben Oct 27, 2023
04f5310
Add comments/phpdoc
datengraben Oct 27, 2023
d1ddb09
Merge branch 'refactoring/upgrade' into bugfix/issue-1273
hansmorb Oct 27, 2023
eaff0e9
Merge branch 'master' into bugfix/issue-1273
hansmorb Oct 30, 2023
e388287
clean up changelist
hansmorb Oct 30, 2023
b4d3932
Merge branch 'master' into bugfix/issue-1273
hansmorb Jan 4, 2024
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
33 changes: 33 additions & 0 deletions src/Migration/Migration.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use CommonsBooking\Wordpress\CustomPostType\Item;
use CommonsBooking\Wordpress\CustomPostType\Location;
use CommonsBooking\Wordpress\CustomPostType\Timeframe;
use CommonsBooking\Wordpress\CustomPostType\CustomPostType;
use Exception;
use WP_Post;
use WP_Query;
Expand Down Expand Up @@ -61,6 +62,11 @@ public static function migrateAll() {
'complete' => 0,
'failed' => 0,
],
'restrictions' => [
'index' => 0,
'complete' => 0,
'failed' => 0,
],
'bookingCodes' => [
'index' => 0,
'complete' => 0,
Expand Down Expand Up @@ -106,6 +112,10 @@ public static function migrateAll() {
'repoFunction' => 'getBookings',
'migrationFunction' => 'migrateBooking',
],
'restrictions' => [
'repoFunction' => 'getRestrictions',
'migrationFunction' => 'migrateRestriction',
],
'bookingCodes' => [
'repoFunction' => 'getBookingCodes',
'migrationFunction' => 'migrateBookingCode',
Expand Down Expand Up @@ -567,6 +577,29 @@ public static function migrateBooking( $booking ): bool {
return self::savePostData( $existingPost, $postData, $postMeta );
}

public static function migrateRestriction( $restriction ): bool {

// Nothing to change for migration
$postData = array();
// Nothing to change for migration
$postMeta = array();

$existingPost = self::getExistingPost( $restriction['id'], \CommonsBooking\Wordpress\CustomPostType\Restriction::$postType );

// Conditionally migrate
$restrictionItems = get_post_meta( $existingPost->ID, \CommonsBooking\Model\Restriction::META_ITEM_ID );
if ( empty( $restrictionItems) ) {
$postMeta[ \CommonsBooking\Model\Restriction::META_ITEM_ID ] = CustomPostType::SELECTION_ALL_POSTS;
}

$restrictionLocations = get_post_meta( $existingPost->ID, \CommonsBooking\Model\Restriction::META_LOCATION_ID );
if ( empty( $restrictionLocations) ) {
$postMeta[ \CommonsBooking\Model\Restriction::META_LOCATION_ID ] = CustomPostType::SELECTION_ALL_POSTS;
}

return self::savePostData( $existingPost, $postData, $postMeta );
}

/**
* Migrates CB1 Booking Code to CB2.
*
Expand Down
14 changes: 12 additions & 2 deletions src/Repository/Restriction.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use CommonsBooking\Helper\Wordpress;
use CommonsBooking\Plugin;
use CommonsBooking\Wordpress\CustomPostType\CustomPostType;
use Exception;

class Restriction extends PostRepository {
Expand Down Expand Up @@ -185,6 +186,10 @@ private static function getActiveQuery(): string {
* WARNING: This method will filter out posts that are only queried by item OR location.
* Meaning, if a restriction is created that has a location and an item, but the query only contains the location, the restriction will not be returned.
*
* TODO:
* - Refactor this method to be more readable and properly document, that having no location set actually means that the restriction applies to all locations.
* - Write migration function to update all restrictions that have no location set to use the new "all" value.
*
* @param array $posts
* @param array $locations
* @param array $items
Expand All @@ -193,13 +198,18 @@ private static function getActiveQuery(): string {
*/
private static function filterPosts( array $posts, array $locations, array $items ): array {
return array_filter( $posts, function ( $post ) use ( $locations, $items ) {
//check, if the restriction has been set to apply to all items or all locations.
//Previously it used to be the case, that if a restriction had no item defined, it would apply to all items and the same for locations.
//This is no longer the case, but we still need to support it for backwards compatibility.
$appliedForAllItems = get_post_meta( $post->ID, \CommonsBooking\Model\Restriction::META_ITEM_ID, true ) === CustomPostType::SELECTION_ALL_POSTS;
$appliedForAllLocations = get_post_meta( $post->ID, \CommonsBooking\Model\Restriction::META_LOCATION_ID, true ) === CustomPostType::SELECTION_ALL_POSTS;
// Check if restriction is in relation to item and/or location
$location = intval( get_post_meta( $post->ID, \CommonsBooking\Model\Restriction::META_LOCATION_ID, true ) );
$restrictionHasLocation = $location !== 0;
$restrictionHasLocation = $location !== 0 | ! $appliedForAllLocations;
$restrictedLocationInLocations = $restrictionHasLocation && in_array( $location, $locations );

$item = intval( get_post_meta( $post->ID, \CommonsBooking\Model\Restriction::META_ITEM_ID, true ) );
$restrictionHasItem = $item !== 0;
$restrictionHasItem = $item !== 0 | ! $appliedForAllItems;
$restrictedItemInItems = $restrictionHasItem && in_array( $item, $items );

// No item or location for restriction set
Expand Down
27 changes: 23 additions & 4 deletions src/Wordpress/CustomPostType/CustomPostType.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ abstract class CustomPostType {
*/
protected $menuPosition;

const SELECTION_ALL_POSTS = 'all_posts';

/**
* @return string
*/
Expand All @@ -43,13 +45,26 @@ public static function getWPNonceId(): string {

/**
* Replaces WP_Posts by their title for options array.
*
* If $allOption is true, an additional option that allows to select all posts is added to the beginning of the array.
* This option is only visible to administrator roles, since usually only they can see all posts.
*
* TODO:
* - All could be interpreted differently depending on the context of the editor. So in the case of a CB Manager "All" could mean all items or locations they manage.
*
* @param $data
* @param WP_Post[]|string[] $data
* @param bool $allOption
*
* @return array
*/
public static function sanitizeOptions( $data ) {
public static function sanitizeOptions( $data, $allOption = false ) {
$options = [];

// Add option to select all items
if ( $allOption && current_user_can( 'administrator' ) ) {
$options[self::SELECTION_ALL_POSTS] = __( 'All', 'commonsbooking' );
Copy link
Contributor

@datengraben datengraben Aug 20, 2023

Choose a reason for hiding this comment

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

An der Stelle könnte man doch auch eine ALLE Option einfügen für die Manager-Rolle, sowas wie All currently managed by you, dann spart man sich das TODO oben

Copy link
Contributor

Choose a reason for hiding this comment

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

Bzw. ich verstehe den Code so das es aktuell nur Administrator-Rollen möglich ist die All Option zu sehen. Das heißt der Fall im Todo ist doch gar nicht möglich, da Manager-Rollen All nie sehen würden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Na ja, theoretisch müsste das ja nicht so sein. Je nachdem welcher post_author gesetzt ist könnte "all" im verschiedenen Kontext was anderes bedeuten. Wenn der post_author zb CB Manager ist und zwei Items zugewiesen hat, wäre all für den post_author diese beiden Items.

}

if ( $data ) {
foreach ( $data as $key => $item ) {
if ( $item instanceof WP_Post ) {
Expand All @@ -62,9 +77,13 @@ public static function sanitizeOptions( $data ) {

$key = $item->ID;
$label = $item->post_title . $statusLabel;
} else {
} elseif ( is_string( $item ) ) {
$label = $item;
}
else {
//All other data types are not supported
continue;
}
$options[ $key ] = $label;
}
}
Expand Down Expand Up @@ -341,4 +360,4 @@ public static function getModel( $post ) {
throw new \Exception('No suitable model found.');
}

}
}
8 changes: 4 additions & 4 deletions src/Wordpress/CustomPostType/Restriction.php
Original file line number Diff line number Diff line change
Expand Up @@ -424,15 +424,15 @@ protected function getCustomFields(): array {
'name' => esc_html__( "Location", 'commonsbooking' ),
'id' => \CommonsBooking\Model\Restriction::META_LOCATION_ID,
'type' => 'select',
'show_option_none' => esc_html__( 'All', 'commonsbooking' ),
'options' => self::sanitizeOptions( \CommonsBooking\Repository\Location::getByCurrentUser() ),
'show_option_none' => esc_html__( '— Please select —', 'commonsbooking' ),
'options' => self::sanitizeOptions( \CommonsBooking\Repository\Location::getByCurrentUser(), true ),
),
array(
'name' => esc_html__( "Item", 'commonsbooking' ),
'id' => \CommonsBooking\Model\Restriction::META_ITEM_ID,
'type' => 'select',
'show_option_none' => esc_html__( 'All', 'commonsbooking' ),
'options' => self::sanitizeOptions( \CommonsBooking\Repository\Item::getByCurrentUser() ),
'show_option_none' => esc_html__( '— Please select —', 'commonsbooking' ),
'options' => self::sanitizeOptions( \CommonsBooking\Repository\Item::getByCurrentUser(), true ),
),
array(
'name' => esc_html__( "Hint", 'commonsbooking' ),
Expand Down
22 changes: 21 additions & 1 deletion tests/Repository/RestrictionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use CommonsBooking\Repository\Restriction;
use CommonsBooking\Tests\Wordpress\CustomPostTypeTest;
use PHPUnit\Framework\TestCase;
use CommonsBooking\Wordpress\CustomPostType\CustomPostType;

class RestrictionTest extends CustomPostTypeTest
{
Expand All @@ -29,6 +29,26 @@ public function testGetByItemAndLocation()
$this->assertEquals( $this->restrictionId, $restrictions[0]->ID );
}

public function testGetSetAll() {
$allRestriction = new \CommonsBooking\Model\Restriction(
$this->createRestriction(
"hint",
CustomPostType::SELECTION_ALL_POSTS,
CustomPostType::SELECTION_ALL_POSTS,
strtotime(self::CURRENT_DATE),
strtotime("+1 day", strtotime(self::CURRENT_DATE)),
)
);
$restrictions = Restriction::get( [$this->locationId], [$this->itemId],null,true);
//make sure that we get both restrictions
$this->assertEquals( 2, count( $restrictions ) );
$restrictionIds = array_map( function( $restriction ) {
return $restriction->ID;
}, $restrictions );
$this->assertContains( $allRestriction->ID, $restrictionIds );
$this->assertContains( $this->restrictionId, $restrictionIds );
}



protected function setUp(): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* parent class for all the custom post type tests.
* This, however extends this class because we need it's methods
*/
class CustomPostTypeTest extends \CommonsBooking\Tests\Wordpress\CustomPostTypeTest
class CustomPostTypeClassTest extends \CommonsBooking\Tests\Wordpress\CustomPostTypeTest
{
protected int $bookingId;
protected Booking $bookingModel;
Expand Down Expand Up @@ -72,6 +72,59 @@ public function testGetModel() {
}

}

public function testSanitizeOptions() {
//create an array of locations and see if it is sanitized correctly
$locationOne = get_post($this->createLocation("Location One",'publish'));
$locationTwo = get_post($this->createLocation("Location Two",'publish'));
$locationThree = get_post($this->createLocation("Location Three",'publish'));
$locationsArray = [$locationOne, $locationTwo, $locationThree];
$locationArray = [
$locationOne->ID => $locationOne->post_title,
$locationTwo->ID => $locationTwo->post_title,
$locationThree->ID => $locationThree->post_title
];
$this->assertEquals($locationArray, CustomPostType::sanitizeOptions($locationsArray));

//now do the same with items
$itemOne = get_post($this->createItem("Item One",'publish'));
$itemTwo = get_post($this->createItem("Item Two",'publish'));
$itemThree = get_post($this->createItem("Item Three",'publish'));
$itemsArray = [$itemOne, $itemTwo, $itemThree];
$itemArray = [
$itemOne->ID => $itemOne->post_title,
$itemTwo->ID => $itemTwo->post_title,
$itemThree->ID => $itemThree->post_title
];
$this->assertEquals($itemArray, CustomPostType::sanitizeOptions($itemsArray));

//and now with an associative array of strings (should return the same array)
$array = [
'one' => 'One',
'two' => 'Two',
'three' => 'Three'
];
$this->assertEquals($array, CustomPostType::sanitizeOptions($array));

//make sure that unsupported post types are not returned
$unsupported = [new \DateTime()];
$this->assertEmpty(CustomPostType::sanitizeOptions($unsupported));

//make sure, that for the admin user all is passed along when requested but not for the other users
$this->createAdministrator();
$this->createSubscriber();
$itemArrayAndAll = [
CustomPostType::SELECTION_ALL_POSTS => 'All',
$itemOne->ID => $itemOne->post_title,
$itemTwo->ID => $itemTwo->post_title,
$itemThree->ID => $itemThree->post_title
];
wp_set_current_user($this->adminUserID);
$this->assertEquals($itemArrayAndAll, CustomPostType::sanitizeOptions($itemsArray, true));
wp_set_current_user($this->subscriberId);
$this->assertEquals($itemArray, CustomPostType::sanitizeOptions($itemsArray, true));
}

protected function setUp(): void {
parent::setUp();
$this->timeframeId = $this->createBookableTimeFrameIncludingCurrentDay();
Expand Down
4 changes: 2 additions & 2 deletions tests/Wordpress/CustomPostTypeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ protected function createSubscriber(){
* In that case, the unit tests would fail, because there is already the user with this ID in the database.
* @return void
*/
public function createAdministrator(){
protected function createAdministrator(){
$wp_user = get_user_by('email',"[email protected]");
if (! $wp_user) {
$this->adminUserID = wp_create_user( "adminuser", "admin", "[email protected]" );
Expand All @@ -301,7 +301,7 @@ public function createAdministrator(){
}
}

public function createCBManager(){
protected function createCBManager(){
//we need to run the functions that add the custom user role and assign it to the user
Plugin::addCustomUserRoles();
//and add the caps for each of our custom post types
Expand Down