-
Notifications
You must be signed in to change notification settings - Fork 5
877 Možnost dát aktivitu do více místností #689
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
base: 877-zahodit-nepotrebne-sql-filtry-aktivity
Are you sure you want to change the base?
The head ref may contain hidden characters: "877-mo\u017Enost-d\u00E1t-aktivitu-do-v\u00EDce-m\u00EDstnost\u00ED-v2"
877 Možnost dát aktivitu do více místností #689
Conversation
85106ea
to
e77d140
Compare
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.
Pull Request Overview
This PR enables assigning each activity to multiple rooms with a designated main room. Key changes include:
- Introduction of a dedicated
Lokace
entity and a join tableakce_lokace
supporting multi-room assignments. - Updates to the
Aktivita
model, templates, import/export logic, and report scripts to handle multiple locations and highlight a primary room. - Database migration script renaming and restructuring the old
akce_lokace
table and moving single-room data into the new join table.
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
model/Aktivita/Lokace.php | New Lokace class representing rooms |
model/Aktivita/Aktivita.php | Extended Aktivita to load, sort, and save multiple rooms |
model/Aktivita/templates/editor-aktivity.xtpl | UI changes: multi-select for rooms and main-room selector |
model/Aktivita/Program.php | Logic for duplicating activities per room and ordering |
migrace/2025-05-27-165452-aktivita-do-vice-mistnosti.sql | Migration: rename old table, create new join table |
admin/scripts/modules/aktivity/*.php & report files | Updated admin modules and reports to query new structure |
Comments suppressed due to low confidence (1)
admin/scripts/zvlastni/reporty/update-zustatku.php:17
- [nitpick] Deleting all rows via
DELETE ... WHERE TRUE
can be slower and log-intensive. UseTRUNCATE TABLE lokace
for faster full-table clears if no foreign key constraints will be violated.
DELETE FROM lokace WHERE TRUE;
@@ -2807,8 +3064,12 @@ public function idStavu(): ?int | |||
/** | |||
* Vrací surový databázový řádek, nepoužívat (pouze pro debug a zpětnou kompatibilitu, postupně odstranit). | |||
*/ | |||
public function rawDb(): array | |||
public function rawDb(array $values = null): array |
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.
The parameter is typed as array
but has a default of null
. This will cause a type error in strict mode. Change the signature to public function rawDb(?array $values = null): array
.
public function rawDb(array $values = null): array | |
public function rawDb(?array $values = null): array |
Copilot uses AI. Check for mistakes.
} | ||
$indexLokace++; |
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.
Incrementing $indexLokace
only once per category group instead of once per location entry will miscalculate last_optgroup_lokace_end
. Move the increment inside the inner loop to track each location correctly.
} | |
$indexLokace++; | |
$indexLokace++; | |
} |
Copilot uses AI. Check for mistakes.
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.
this ^
$serazeneRozkopirovane = []; | ||
foreach ($aktivityPodleDnuALokaci as $aktivityPodleLokaci) { | ||
foreach ($serazenaIdsLokaci as $idLokace) { | ||
$serazeneRozkopirovane = [...$serazeneRozkopirovane, ...$aktivityPodleLokaci[$idLokace] ?? []]; |
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.
Using the spread operator in a loop recreates the array on each iteration, leading to O(n²) behavior for large sets. Consider using array_merge
once per day group or pushing items directly to avoid repeated copying.
$serazeneRozkopirovane = [...$serazeneRozkopirovane, ...$aktivityPodleLokaci[$idLokace] ?? []]; | |
$serazeneRozkopirovane = array_merge($serazeneRozkopirovane, $aktivityPodleLokaci[$idLokace] ?? []); |
Copilot uses AI. Check for mistakes.
f519cfd
to
96193f2
Compare
96193f2
to
d51b83b
Compare
@@ -951,7 +951,7 @@ private function zapoctiZustatekZPredchozichRocniku(): void | |||
private function aplikujBonusZaVedeniAktivit(float $cena): float | |||
{ | |||
$zbyvajiciBonusZaVedeniAktivit = $this->bonusZaVedeniAktivit(); | |||
$zbyvajiciCena = $cena; | |||
$zbyvajiciCena = $cena; |
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.
What's the point?
model/Cache/CachedDb.php
Outdated
$tableVersions = dbFetchPairs(<<<SQL | ||
$tableVersions = dbFetchPairs(<<<SQL | ||
SELECT table_name, version | ||
FROM _table_data_versions | ||
WHERE table_name IN ($0) | ||
SQL, | ||
[0 => $relatedTables], | ||
); | ||
[0 => $relatedTables], | ||
); |
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.
One more level of indent please
|
||
CREATE TABLE akce_lokace | ||
( | ||
id_akce_lokace SERIAL, |
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.
SERIAL je, co se mi daří dohledat, spíš nedoporučovaný jako typ
id_akce_lokace SERIAL, | |
id_akce_lokace INT UNSIGNED AUTO_INCREMENT NOT NULL UNIQUE, |
UPDATE akce_seznam SET lokace=NULL WHERE TRUE; | ||
TRUNCATE TABLE akce_lokace; -- smazat všechny místnosti, aby se mohly nahrát každý rok znovu a nehrozilo, že to někdo začne zadávat k aktivitám, když to ještě není nahrané | ||
-- smazat všechny místnosti, aby se mohly nahrát každý rok znovu a nehrozilo, že to někdo začne zadávat k aktivitám, když to ještě není nahrané | ||
DELETE FROM akce_lokace WHERE TRUE; | ||
DELETE FROM lokace WHERE TRUE; |
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.
Nebylo by lepší mít rovnou u lokací i rok a nemuset si mazat každý rok kus databáze, když už se do toho stejně hrabe?
dbQueryS('UPDATE akce_lokace SET poradi=poradi-1 WHERE poradi=$0', [post('poradi') + 1]); | ||
dbQueryS('UPDATE akce_lokace SET poradi=poradi+1 WHERE id_lokace=$0', [post('dolu')]); | ||
dbQueryS('UPDATE lokace SET poradi=poradi-1 WHERE poradi=$0', [post('poradi') + 1]); | ||
dbQueryS('UPDATE lokace SET poradi=poradi+1 WHERE id_lokace=$0', [post('dolu')]); |
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.
Nebylo by toto vhodné přepsat tak aby to nespoléhalo že bude správná hodnota i v post("poradi") i v post("dolu"), tzn. aby to používalo pouze jednu z těchto hodnot a druhou si to dopočítalo samo na backendu? Ideálně i dát do jednoho dotazu, něco jako UPDATE lokace SET poradi= CASE WHEN ... WHEN ..., nebo zabalit do db transakce
?int $hlavniLokaceId, | ||
): void { | ||
if ($lokaceIds === [] && $hlavniLokaceId === null) { | ||
return; |
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.
Tohle nedává příliš smysl, pokud se metoda jmenuje chybaNebylaLiHlavníLokaceNastavena
, tak by absence hlavní lokace měla vyvolat chybu...
}, $this->seznamLokaci())); | ||
} | ||
|
||
public function nazevLokaci(): string |
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.
nazvyLokaci
return ImportStepResult::error($locationAccessibilityResult->getError()); | ||
} | ||
$checkResults[] = $locationAccessibilityResult; | ||
unset($locationAccessibilityResult); |
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.
Why the unset?
if ($seznamLokaciIdcka === []) { | ||
$aktivityPodleDnuALokaci[$den][0][$aktivita->id()] = $aktivita; | ||
} else { | ||
foreach ($aktivita->seznamLokaciIdcka() as $idLokace) { |
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.
foreach ($aktivita->seznamLokaciIdcka() as $idLokace) { | |
foreach ($seznamLokaciIdcka as $idLokace) { |
@@ -558,7 +603,8 @@ private function tiskObsahuTabulky( | |||
?array &$aktivitaRaw, | |||
$denId = null, | |||
): void { | |||
$pocetAktivit = 0; | |||
$pocetAktivit = 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.
What's the point of this indent?
e77d140
to
35b9027
Compare
https://trello.com/c/1xy2J2rg/877-mo%C5%BEnost-d%C3%A1t-aktivitu-do-v%C3%ADce-m%C3%ADstnost%C3%AD
Náhrada za #671