-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: master
Are you sure you want to change the base?
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. |
|
||
// 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.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hier @deprecated
und vielleicht ein TODO für nächstes minor release 2.9 oder 3.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nur eine Kleinigkeit.
@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: