-
Notifications
You must be signed in to change notification settings - Fork 58
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
Mass delete templates in container version #522
base: 4.x-dev
Are you sure you want to change the base?
Conversation
* @param int $idContainerVersion | ||
* @param string $deletedDate | ||
*/ | ||
public function deleteContainerTags($idSite, $idContainerVersion, $deletedDate) |
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.
@ulcuber You aiming to update Import.php too with this PR right ?
Can you add those changes too ?
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.
I have tried to split it into small atomic independent issues. I'll optimize import next week
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.
@ulcuber Its better to include all such changes in 1 PR so that we can test and ensure that new changes do not lead to any regressions, I would suggest to add the Importer changes in same PR too or folk a branch from this and add those changes if you want to keep your changes separately
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.
Added comment for complete changes
@AltamashShaikh added optimization in import. |
$this->variables->deleteContainerVariable($idSite, $idContainerVersion, $variable['idvariable']); | ||
} | ||
$now = Date::now()->getDatetime(); | ||
$this->tagsDao->deleteContainerTags($idSite, $idContainerVersion, $now); |
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.
@ulcuber Can you create a deleteContainerTags()
in Model/Tag.php
and call the Model instead of dao
} | ||
$now = Date::now()->getDatetime(); | ||
$this->tagsDao->deleteContainerTags($idSite, $idContainerVersion, $now); | ||
$this->triggersDao->deleteContainerTriggers($idSite, $idContainerVersion, $now); |
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.
Same here call deleteContainerTriggers()
in Model/Trigger.php
and call the Model instead of dao
$now = Date::now()->getDatetime(); | ||
$this->tagsDao->deleteContainerTags($idSite, $idContainerVersion, $now); | ||
$this->triggersDao->deleteContainerTriggers($idSite, $idContainerVersion, $now); | ||
$this->variablesDao->deleteContainerVariables($idSite, $idContainerVersion, $now); |
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.
Same here Same here call deleteContainerVariables()
in Model/Variable.php
and call the Model instead of dao
@@ -119,17 +156,10 @@ public function importContainerVersion($exportedContainerVersion, $idSite, $idCo | |||
{ | |||
$this->checkImportContainerIsPossible($exportedContainerVersion, $idSite, $idContainer); | |||
|
|||
foreach ($this->tags->getContainerTags($idSite, $idContainerVersion) as $tag) { |
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.
@tsteur any specific reason we deleted 1 tag within a container at a time instead of deleting all at once like in this PR ?
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.
@ulcuber left few comments
Description:
Allows to resolve n+1 queries when deleting all templates in version
Review