Behebe "Alle" Option verfügbar für CB Manager #1306
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1306 +/- ##
============================================
- Coverage 41.27% 38.11% -3.17%
+ Complexity 2329 2198 -131
============================================
Files 91 85 -6
Lines 9613 8866 -747
============================================
- Hits 3968 3379 -589
+ Misses 5645 5487 -158 ☔ View full report in Codecov by Sentry. |
hansmorb
added
bug
|
|
||
| // Add option to select all items | ||
| if ( $allOption && current_user_can( 'administrator' ) ) { | ||
| $options[self::SELECTION_ALL_POSTS] = __( 'All', 'commonsbooking' ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@hansmorb können wir in 'cpt/Restriction:savePost` (ab Zeile 522) nicht noch den Fall mit 'Please Select' und der Manager-Rolle abfangen? Dann wäre es nicht möglich als Manager den ALLE-Fall zu triggern.
This adds migration for applied locations and items meta (but its untested)
src/Model/Restriction.php
Outdated
| @@ -238,50 +238,68 @@ public function getEndDateDateTime(): DateTime { | |||
|
|
|||
| /** | |||
| * Returns item name for the item that is restricted. | |||
| * | |||
| * DEPRECATED: This is not used anywhere in the code. Besides, the function is broken since it does not consider the case where multiple items are selected. | |||
There was a problem hiding this comment.
Hier @deprecated und vielleicht ein TODO für nächstes minor release 2.9 oder 3.0
|
@datengraben Danke für dein Review. Ich habe, als ich mir das ganze nochmal genauer angeschaut habe festgestellt, dass ich einige Änderungen nicht so konsequent durchgezogen habe, deshalb habe ich hier nochmal einiges geändert. Ich werde wahrscheinlich auch das Refactoring des Upgrade Prozesses in einen separaten PR auslagern. |
# Conflicts: # src/Service/Upgrade.php # tests/php/Service/UpgradeTest.php
the unit tests worked because $previousVersion = '' is also != to any other version
# Conflicts: # src/Service/Upgrade.php
# Conflicts: # src/Service/Upgrade.php # tests/php/Service/UpgradeTest.php
|
Ich würde den MR gerne vertagen, weil ich komische Fehler auf dem Branch bekomme, wenn ich die Unit Tests einzeln aufrufe findet er zwar die Klasse aber kann die Migration nicht durchführen. Das ist mir für den nächsten Release gerade noch zu heikel |
# Conflicts: # src/Service/Upgrade.php
Danke @genevievecory . Ich habe hier die Logik etwas umgebastelt. Ursprünglich war es so, dass diese "Alle" Option quasi der "keine" Option entsprochen hat. Also wenn der Wert leer war, wurde davon ausgegangen, dass es auf Alle gesetzt ist. Jetzt muss ein spezifischer "Alle" Meta Key gesetzt werden der auch nur für Admins sichtbar ist.
Potentielle Probleme:
TODOS: