Skip to content
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

Stands Controller with view of existing stands Report Controller Fix some issues #216

Merged
merged 2 commits into from
Jan 1, 2025

Conversation

sveneld
Copy link
Collaborator

@sveneld sveneld commented Jan 1, 2025

PR Type

Enhancement


Description

  • Migrate stand and stats to new API controllers

  • Add new report and stand management features

  • Enhance admin UI with improved tables and cards

  • Add input validation and requirements to routes


Changes walkthrough 📝

Relevant files
Enhancement
9 files
actions-web.php
Remove deprecated stand and stats functions                           
+0/-50   
command.php
Remove deprecated API endpoints                                                   
+0/-18   
routes.php
Add new API routes and requirements                                           
+19/-2   
ReportController.php
Add new Report Controller for stats                                           
+68/-0   
StandController.php
Add new Stand Controller for stand management                       
+38/-0   
HistoryRepository.php
Add daily and user stats repository methods                           
+38/-0   
admin.js
Update admin UI for stands and reports                                     
+116/-34
report.html.twig
Add new report template with tables                                           
+56/-0   
stands.html.twig
Add new stands template with cards                                             
+29/-0   
Additional files
11 files
AdminController.php +1/-0     
BikeController.php +1/-1     
UserController.php +2/-2     
PersonalStatsController.php +11/-2   
ScanController.php +2/-1     
ControllerEventListener.php +3/-0     
StandRepository.php +2/-1     
index.html.twig +2/-16   
user.edit.html.twig +38/-0   
user.html.twig +1/-36   
messages+intl-icu.en.php +6/-0     

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Report Controller
Fix some issues
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

SQL Injection:
The HistoryRepository.php file contains a SQL query that directly concatenates user input (year parameter) into the query string: "AND YEAR(time) = " . $year. This creates a potential SQL injection vulnerability. The query should use parameterized statements instead.

⚡ Recommended focus areas for review

Input Validation

The year parameter validation in user() method has hardcoded values (1900, 2010) that should be configurable constants. The validation logic could also be moved to a separate validation service.

if ($year === 1900) {
    $year = (int)date('Y');
} elseif (
    $year > (int)date('Y')
    || $year < 2010
) {
    return $this->json([], Response::HTTP_BAD_REQUEST);
}
SQL Injection Risk

The SQL query in userStats() method directly concatenates the year parameter into the query string without proper escaping or parameterization.

AND YEAR(time) = " . $year . "
Error Handling

The AJAX error handling in the stands() function is incomplete - it catches errors but doesn't provide user feedback or fallback behavior when the API call fails.

error: function(xhr, status, error) {
    console.error("Error fetching stand data:", error);
}

Copy link
Contributor

CI Failure Feedback 🧐

Action: phpcs (7.4)

Failed stage: Run PHP CodeSniffer [❌]

Failed test name: phpcs

Failure summary:

The PHP Code Sniffer (PHPCS) check failed due to multiple coding standard violations:

  • In StandController.php: Incorrect formatting of multi-line function declaration (closing parenthesis
    and opening brace must be on same line)
  • In ReportController.php and PersonalStatsController.php: Arguments with default values not placed at
    the end of argument lists
  • In HistoryRepository.php: Two instances where closing parenthesis of multi-line function calls must
    be on separate lines

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    177:  - Locking symfony/asset (v5.4.45)
    178:  - Locking symfony/cache (v5.4.46)
    179:  - Locking symfony/cache-contracts (v2.5.4)
    180:  - Locking symfony/config (v5.4.46)
    181:  - Locking symfony/console (v5.4.47)
    182:  - Locking symfony/dependency-injection (v5.4.48)
    183:  - Locking symfony/deprecation-contracts (v2.5.4)
    184:  - Locking symfony/dotenv (v5.4.48)
    185:  - Locking symfony/error-handler (v5.4.46)
    ...
    
    281:  - Downloading symfony/var-exporter (v5.4.45)
    282:  - Downloading symfony/cache (v5.4.46)
    283:  - Downloading symfony/expression-language (v5.4.45)
    284:  - Downloading symfony/routing (v5.4.48)
    285:  - Downloading symfony/polyfill-php81 (v1.31.0)
    286:  - Downloading symfony/http-foundation (v5.4.48)
    287:  - Downloading symfony/event-dispatcher (v5.4.45)
    288:  - Downloading symfony/var-dumper (v5.4.48)
    289:  - Downloading symfony/error-handler (v5.4.46)
    ...
    
    367:  - Installing symfony/var-exporter (v5.4.45): Extracting archive
    368:  - Installing symfony/cache (v5.4.46): Extracting archive
    369:  - Installing symfony/expression-language (v5.4.45): Extracting archive
    370:  - Installing symfony/routing (v5.4.48): Extracting archive
    371:  - Installing symfony/polyfill-php81 (v1.31.0): Extracting archive
    372:  - Installing symfony/http-foundation (v5.4.48): Extracting archive
    373:  - Installing symfony/event-dispatcher (v5.4.45): Extracting archive
    374:  - Installing symfony/var-dumper (v5.4.48): Extracting archive
    375:  - Installing symfony/error-handler (v5.4.46): Extracting archive
    ...
    
    418:  shell: /usr/bin/bash -e {0}
    419:  env:
    420:  COMPOSER_PROCESS_TIMEOUT: 0
    421:  COMPOSER_NO_INTERACTION: 1
    422:  COMPOSER_NO_AUDIT: 1
    423:  ##[endgroup]
    424:  FILE: ...SourceBikeShare/OpenSourceBikeShare/src/Controller/Api/StandController.php
    425:  --------------------------------------------------------------------------------
    426:  FOUND 1 ERROR AFFECTING 1 LINE
    427:  --------------------------------------------------------------------------------
    428:  22 | ERROR | [x] The closing parenthesis and the opening brace of a multi-line
    429:  |       |     function declaration must be on the same line
    430:  --------------------------------------------------------------------------------
    431:  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    432:  --------------------------------------------------------------------------------
    433:  FILE: ...ourceBikeShare/OpenSourceBikeShare/src/Controller/Api/ReportController.php
    434:  --------------------------------------------------------------------------------
    435:  FOUND 1 ERROR AFFECTING 1 LINE
    436:  --------------------------------------------------------------------------------
    437:  42 | ERROR | Arguments with default values must be at the end of the argument
    438:  |       | list
    439:  --------------------------------------------------------------------------------
    440:  FILE: ...ceBikeShare/OpenSourceBikeShare/src/Controller/PersonalStatsController.php
    441:  --------------------------------------------------------------------------------
    442:  FOUND 1 ERROR AFFECTING 1 LINE
    443:  --------------------------------------------------------------------------------
    444:  21 | ERROR | Arguments with default values must be at the end of the argument
    445:  |       | list
    446:  --------------------------------------------------------------------------------
    447:  FILE: ...enSourceBikeShare/OpenSourceBikeShare/src/Repository/HistoryRepository.php
    448:  --------------------------------------------------------------------------------
    449:  FOUND 2 ERRORS AFFECTING 2 LINES
    450:  --------------------------------------------------------------------------------
    451:  49 | ERROR | [x] Closing parenthesis of a multi-line function call must be on
    452:  |       |     a line by itself
    453:  69 | ERROR | [x] Closing parenthesis of a multi-line function call must be on
    454:  |       |     a line by itself
    455:  --------------------------------------------------------------------------------
    456:  PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    457:  --------------------------------------------------------------------------------
    458:  Time: 2.03 secs; Memory: 12MB
    459:  ##[error]Process completed with exit code 2.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    Copy link
    Contributor

    qodo-merge-pro-for-open-source bot commented Jan 1, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Prevent SQL injection vulnerability by using parameterized queries instead of string concatenation

    The SQL query in userStats() is vulnerable to SQL injection as the year parameter is
    directly concatenated into the query. Use parameterized queries instead.

    src/Repository/HistoryRepository.php [66]

    -"AND YEAR(time) = " . $year . "
    +"AND YEAR(time) = ?" 
    +)->fetchAllAssoc([$year]);
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Direct string concatenation in SQL queries creates a serious security vulnerability. Using parameterized queries is essential for preventing SQL injection attacks.

    10
    Possible issue
    ✅ Fix invalid route annotation syntax that would prevent proper routing
    Suggestion Impact:The commit fixed the route annotation syntax by removing the duplicate comma and correcting the requirements syntax exactly as suggested

    code diff:

    -     * @Route("/report/user/{year}", name="api_report_user", , requirements: ['year' => '\d+'] methods={"GET"})
    +     * @Route("/report/user/{year}", name="api_report_user", requirements: {'year' => '\d+'}, methods={"GET"})

    Fix the incorrect syntax in the route annotation for the user method. The comma
    after 'api_report_user' is duplicated and will cause routing issues.

    src/Controller/Api/ReportController.php [38]

    -@Route("/report/user/{year}", name="api_report_user", , requirements: ['year' => '\d+'] methods={"GET"})
    +@Route("/report/user/{year}", name="api_report_user", requirements: {'year' => '\d+'}, methods={"GET"})
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The duplicate comma in the route annotation is a critical syntax error that would prevent the route from working correctly, potentially breaking the API endpoint functionality.

    9
    General
    ✅ Use a more appropriate default value for the year parameter to avoid unnecessary validation logic
    Suggestion Impact:The commit implements the suggested change by replacing the default year 1900 with null and adding logic to set it to current year when null

    code diff:

    -        $year = 1900,
             HistoryRepository $historyRepository,
    -        LoggerInterface $logger
    +        LoggerInterface $logger,
    +        $year = null
         ): Response {
             if (!$this->isGranted('ROLE_ADMIN')) {
                 $logger->info(
    @@ -52,7 +52,7 @@
     
                 return $this->json([], Response::HTTP_FORBIDDEN);
             }
    -        if ($year === 1900) {
    +        if (is_null($year)) {
                 $year = (int)date('Y');
             } elseif (

    The default year value of 1900 is not a valid year for the system (which accepts
    2010 onwards). Change the default to use the current year directly.

    src/Controller/Api/ReportController.php [41-44]

     public function user(
    -    $year = 1900,
    +    $year = null,
         HistoryRepository $historyRepository,
         LoggerInterface $logger
     ): Response {
    +    if ($year === null) {
    +        $year = (int)date('Y');
    +    }
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The current default value of 1900 requires additional validation logic and would never be valid. Using null with proper handling would make the code more maintainable and logical.

    6

    Report Controller
    Fix some issues
    @sveneld sveneld added this to the Symfony Migration milestone Jan 1, 2025
    @sveneld sveneld merged commit 8bfd549 into main Jan 1, 2025
    2 checks passed
    @sveneld sveneld deleted the stats_controller branch January 1, 2025 21:10
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant