Open
Conversation
… clean up unused lines in repositories
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses SonarQube findings across the Symfony (PHP) backend and the TypeScript frontend by tightening error handling, removing dead code, reducing duplication, and applying small refactors to satisfy static analysis.
Changes:
- Replace generic exceptions with domain-specific exceptions (Storage/Parse/FFTA) and propagate updated
@throwsdocs. - Refactor/cleanup code to reduce duplication and “code smell” (constants for repeated strings/selectors, extracted helper logic, remove commented scaffolding).
- Frontend lint/static-analysis tweaks (ES target bump, safer globals usage, minor refactors in Stimulus controllers and utilities).
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Bumps TS compilation target to ES2021. |
| src/Twig/PublicUrlExtension.php | Throws StorageException instead of generic \Exception. |
| src/Twig/AttachmentTemporaryUrlExtension.php | Throws StorageException for temp URL failures. |
| src/Scrapper/ResultArcParser.php | Throws ParseException instead of generic \Exception. |
| src/Scrapper/FftaScrapper.php | Introduces selector constant; throws FftaException for request failure. |
| src/Repository/SightAdjustmentRepository.php | Removes commented template methods. |
| src/Repository/ResultRepository.php | Removes commented template methods. |
| src/Repository/PracticeAdviceRepository.php | Removes commented template methods. |
| src/Repository/LicenseeRepository.php | Removes commented template methods. |
| src/Repository/LicenseeAttachmentRepository.php | Removes commented template methods. |
| src/Repository/LicenseRepository.php | Removes commented template methods. |
| src/Repository/GroupRepository.php | Removes commented template methods. |
| src/Repository/EventParticipationRepository.php | Removes commented template methods. |
| src/Repository/BowRepository.php | Removes commented template methods. |
| src/Repository/ArrowRepository.php | Removes commented template methods. |
| src/Helper/StringHelper.php | Refactors password generation to functional style. |
| src/Helper/ObjectComparator.php | Adds NOSONAR suppression for intentional loose comparison. |
| src/Helper/LicenseHelper.php | Extracts birthdate/dateKey matching into a dedicated method. |
| src/Helper/FftaHelper.php | Refactors licensee sync flow; extracts methods; updates exceptions/docs. |
| src/Exception/StorageException.php | Adds new domain exception. |
| src/Exception/ParseException.php | Adds new domain exception. |
| src/Exception/FftaException.php | Adds new domain exception. |
| src/Entity/Result.php | Minor refactors in parsing and distance/size logic (but introduces a syntax error). |
| src/Entity/Event.php | Extracts date format string into a constant. |
| src/Controller/ResetPasswordController.php | Refactor to reduce early return / simplify flow. |
| src/Controller/RegistrationController.php | Condenses null/id handling into a single assignment. |
| src/Controller/GroupManagementController.php | Refactor to avoid duplicate returns by reusing a response variable. |
| src/Controller/Admin/EventManagementController.php | Removes unused GroupRepository injection/import. |
| src/Controller/Admin/AdminDashboardController.php | Extracts repeated icon class into a constant. |
| src/Command/ResultArcImportCommand.php | Consolidates validation into a single conditional branch. |
| src/Command/DiscordBotRunCommand.php | Simplifies event callback signature. |
| assets/pdf.ts | Minor JS/TS best-practice tweaks (new Error, parseFloat usage, etc.). |
| assets/custom-svg-icons/faBowClassicRecurve.js | Replaces var with const. |
| assets/cookie-consent.ts | Improves typings and uses globalThis for Matomo queue. |
| assets/controllers/results-chart_controller.ts | Names controller class; removes unused locals. |
| assets/controllers/modal_controller.ts | Names controller class; refactors null checks / early return; target declaration tweak. |
| assets/controllers/event-participation_controller.ts | Names controller class; fixes equality check / UI toggle logic. |
| assets/app.ts | Removes commented/dead code. |
| assets/apiCore.ts | Simplifies query string assembly using flat(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+180
to
+191
| $this->entityManager->beginTransaction(); | ||
| $this->entityManager->persist($licensee); | ||
|
|
||
| if ($dbProfilePicture && !$fftaProfilePicture instanceof LicenseeAttachment) { | ||
| $this->logger->notice(' - Removing profile picture'); | ||
| $licensee->removeAttachment($dbProfilePicture); | ||
| $this->entityManager->remove($dbProfilePicture); | ||
| $syncResult = SyncReturnValues::UPDATED; | ||
| } | ||
| try { | ||
| $this->emailHelper->sendWelcomeEmail($licensee, $club); | ||
| } catch (TransportExceptionInterface $transportException) { | ||
| $this->entityManager->rollback(); | ||
|
|
||
| if (!$dbProfilePicture && $fftaProfilePicture instanceof LicenseeAttachment) { | ||
| $this->logger->notice(' + Adding profile picture'); | ||
| $licensee->addAttachment($fftaProfilePicture); | ||
| $licensee->setUpdatedAt(new \DateTimeImmutable()); | ||
| $this->entityManager->persist($fftaProfilePicture); | ||
| $syncResult = SyncReturnValues::UPDATED; | ||
| } | ||
| throw $transportException; | ||
| } | ||
|
|
||
| if (!$dbProfilePicture && !$fftaProfilePicture instanceof LicenseeAttachment) { | ||
| $this->logger->notice(' ! No profile picture'); | ||
| } | ||
| $this->entityManager->commit(); |
There was a problem hiding this comment.
createAndPersistNewLicensee() starts a DB transaction and calls commit()/rollback(), but it never flushes inside the transaction. As a result, the commit commits an empty transaction and the later flush() in syncLicenseeWithId() will write outside the transaction; additionally, on rollback the UnitOfWork still contains scheduled inserts (user/licensee/attachment) that could be flushed later. Use a proper transactional wrapper (e.g., EntityManager::wrapInTransaction) or explicitly flush() before commit(), and ensure the UnitOfWork is cleared/closed on rollback so the new entities can’t be persisted after a failed welcome email.
…ecessary variable assignment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Very dirty PR where I test Claude Sonnet 4.6 by giving him all the Sonarqube issues and let him fix all of them.
Needs careful review and maybe some rollback.