fix(notificationtarget): handle duplicate targets when deleting notification recipients

[MyvTsv]

• I have read the CONTRIBUTING document.
• I have performed a self-review of my code.
• I have added tests that prove my fix is effective or that my feature works.
• This change requires a documentation update.

Description

• It fixes !41780
• Fixes an error when deleting a duplicate recipient for the same notification.

NotificationTargetTicket::getFromDBByCrit() expects to get one result, 2 found in query "SELECT id FROM glpi_notificationtargets WHERE glpi_notificationtargets.notifications_id = '83' AND glpi_notificationtargets.items_id = '3' AND glpi_notificationtargets.type = '1'". In ./src/CommonDBTM.php(437) #0 ./src/NotificationTarget.php(160): CommonDBTM->getFromDBByCrit() #1 ./src/NotificationTarget.php(551): NotificationTarget->getFromDBForTarget() #2 ./front/notificationtarget.form.php(40): NotificationTarget::updateTargets() #3 ./src/Glpi/Controller/LegacyFileLoadController.php(64): require('/mnt/diskhome/h...') #4 ./vendor/symfony/http-kernel/HttpKernel.php(181): Glpi\Controller\LegacyFileLoadController->__invoke() #5 ./vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw() #6 ./vendor/symfony/http-kernel/Kernel.php(197): Symfony\Component\HttpKernel\HttpKernel->handle() #7 ./public/index.php(70): Symfony\Component\HttpKernel\Kernel->handle() #8 {main}

Screenshots (if appropriate):

[Rom1-B]

Maybe we should add an SQL unique key?
In that case, we would need to clean up any potential duplicates during migration, just before adding this key.

[stonebuzz]

I agree with Rom1; the issue can be resolved simply by adding a uniqueness key.

The root cause of the problem lies in an internal SQL script used to add notifications/targets to instances. This script does not raise an error when attempting to add a target that already exists in GLPI, which results in duplicate entries in the database.

[Rom1-B]

It doesn't seem that the error comes from the GLPI basic data, but from a hack on the instance, so I doubt we need to delete anything here?

[MyvTsv]

I removed the target because it was inserted twice (L4380 and L4610). I have to remove it, otherwise I get an error when installing a database.

[Rom1-B]

OK. However, that leaves a gap in the IDs.

Please correct the errors reported by the CIs and it should be OK.

[Rom1-B]

OK. However, that leaves a gap in the IDs.

Please correct the errors reported by the CIs and it should be OK.

[Rom1-B]

According to getUselessKeys(), called by the --detect-useless-keys option, the index on notifications_id should be removed. This is what's causing the CI to fail. However, this index is relevant because some queries filter only by notifications_id, without using other columns. These queries will lose performance, but the impact is only measurable from 10,000 rows onward, which is likely rare. So, we can remove it, unless we prefer to modify getUselessKeys() to satisfy the CI.

Suggested change

KEY `notifications_id` (`notifications_id`),

What do you think? @cedric-anne @AdrienClairembault @trasher

[trasher]

Yes, KEY must be removed since it"s included in unicity.

[trasher]

It must be removed in the migration script as well

[AdrienClairembault]

https://dev.mysql.com/doc/refman/8.4/en/multiple-column-indexes.html

If the table has a multiple-column index, any leftmost prefix of the index can be used by the optimizer to look up rows. For example, if you have a three-column index on (col1, col2, col3), you have indexed search capabilities on (col1), (col1, col2), and (col1, col2, col3).

[AdrienClairembault]

Tests are failing.

[trasher]

Tests are failing.

Yes, because of #22942 (comment)