Merge pull request #36 from ingress-it-solutions/dev

Laravel Specific

action.yaml

@@ -27,54 +27,87 @@ inputs:
     default: 'github'
   PROMPT_TEMPLATE:
     description: 'The template for the FULL_REVIEW_COMMENT prompt.'
-    default: 'Your task is to act as a code reviewer and review a pull request by summarizing the changes made, identifying potential issues related to logic and runtime, and creating a bullet list of action items needed before the change can be approved. The output should focus on items mentioned in the given code review checklist.
+    default: "Your task is to act as a code reviewer and review a pull request by summarizing the changes made, identifying potential issues related to logic and runtime, and creating a bullet list of action items needed before the change can be approved. The output should focus on items mentioned in the given code review checklist.
     Instructions:
-    - Review the output of git diff for the pull request
+
-    - Summarize the overview of the changes made
+- Review the output of git diff for the pull request
-    - Identify potential issues related to logic and runtime
+- Identify potential issues related to logic and runtime
-    - Output as a markdown document, with the following sections:
+- Output as a markdown document, with the following sections:
-        #### Overview of changes:
+    #### Overall Summary:
-          - Summarize the overview of the changes made
+      - Each bullet point should provide the summary on what this Pull request does. Each point shouldn't exceed more than 14 words.
-        #### Changelog:
+    #### Issues & Action Items:
-          - Summarize the overview in a bullet point to consider it in Change-log.  
+      - Title of the check that it violated and a one liner guideline to user on how it can be fixed.
-        #### issues:
+      - TODO List with the name of the file, violating policy and line no where you think it was violated.
-          - Identify potential issues related to logic and runtime
+    #### APPROVE / DISAPPROVE this PR:
-          - Identify issues mentioned in the code review checklist
+      - Tell me if this Pull request can be approved or not. if its disapproved just mention it clearly to fix the problem and raise another pull request. If its approved then appreciate everyone and say it can be approved.
-        #### Action items:
+- If there are no issues, output 'None'
-          - Mandatory action items that are must and needed before the change can be approved
+- If there are no action items, output 'None'
-        #### Joke about this PR:
+- Create a bullet list of action items needed before the change can be approved
-          - Tell me a joke about this Code Review  
+- The response sentences are no longer than 16 words each
-    - If there are no issues, output "None"
+- Keep the response sentences as short as possible
-    - If there are no action items, output "None"
+- Check for N+1 query detection:
-    - Create a bullet list of action items needed before the change can be approved
+  Anytime in a pull request you see that there are foreach loop or call a Collection method you will look for potential N+1 problems.
-    - The response sentences are no longer than 16 words each
+    Here are the list of things that qualifies as a problem:
-    - Keep the response sentences as short as possible
+    - Access a relationship that is not eager-loaded either in the body of the current function (using with() or load()) or in the model itself (using the $with property).
-    - Focus on items mentioned in the given code review checklist:
+    - A call to DB functions in the loop such as DB::table()
-        Code Structure
+    - A call to static Model functions in the loop such as Product::find()
-          - Validation
+    - A call to Model functions in the loop such as $product->save()
-          - Business logic should be in service class
+- Check for Missing whenLoaded() calls:
-          - Dont repeat yourself (DRY)
+  If a pull request shows push an HTTP resource class you will check if pull request code used the whenLoaded() helper provided by Laravel. It's a great way to avoid N+1 and performance issues.
-          - Prefer to use Eloquent over using Query Builder and raw SQL queries. Prefer collections over arrays
+- Check for Missing DB index in migration:
-          - Mass assignment
+  If a pull request code push a migration we check if it has added any kind of index to new columns. The full path has to include migration in order to trigger this check.
-          - Do not execute queries in Blade templates and use eager loading (N + 1 problem)
+- Check for Missing down method in migration:
-          - Chunk data for data-heavy tasks
+  If a pull request code push a migration we check if it has a down method and it's not empty. The full path has to include migration in order to trigger this check.
-          - Comment your code, but prefer descriptive method and variable names over comments
+- Check for Missing foreign key in migration:
-          - Do not put JS and CSS in Blade templates and do not put any HTML in PHP classes
+  If a pull request code create a new column that ends with _id you will warn if a pull request code forgot to add a foreign key to that column.
-          - Use config and language files, constants instead of text in the code
+- Check for Missing authorization in request:
-          - Use standard Laravel tools accepted by community
+  If a pull request code push an HTTP request we check if it has an authorize method and it's not return true. The filename must end with Request.php or the full path has to include Requests in order to trigger this check.
-          - Follow Laravel naming conventions
+- Check for Validation in controller:
-          - Use shorter and more readable syntax where possible
+  If a pull request code push a controller we check if it uses any kind of validator. Usually, it's a better idea to move this logic to a Request class. The filename must end with Controller.php or the full path has to include Controllers in order to trigger this check.
-          - Use IoC container or facades instead of new Class
+- Check for Missing ENV variable:
-          - Are there any unnecessary files, folders, or code modules?
+  If a pull request code added a new key to one of the config file we check if a pull request code also included it in the .env.example file
-          - Does the code follow the Single Responsibility Principle (SRP) and Dont Repeat Yourself (DRY) principle?
+- Check for env() call outside of config files:
-        Error Handling
+  If a pull request code contains an env() call anywhere outside of a config file it needs to be warn. It's a best practice to only use env() in config files.
-          - Are all error scenarios covered in the code?
+- Check for Forgotten cache keys New:
-          - Are the error messages clear and helpful?
+  If a pull request code contains a Cache::rememberForever() call It needs to be warn.
-          - Is the code handling errors gracefully?
+- Check for Validation with reference to Request that has non nullable database table column in migration.  
-        Security
+- Check for Incorrect dependencies:
-          - Are sensitive data and credentials stored securely?
+  There are different layers in every Laravel application. Layers such as: HTTP, Business Logic, Database, etc. Each layer has its own dependencies. For example, the database layer should not depend on the HTTP layer. If it does, it should warn.
-          - Are all external libraries and packages up-to-date?
+    Here are what counts as an incorrect dependency:
-          - Is the code protected against common security vulnerabilities such as SQL injection and cross-site scripting (XSS)?
+    - This class -> Depends on these
+    - Model -> HTTP, Job, Command, Checker
+    - Job -> HTTP
+    - Command -> HTTP
+    - Mail/Notification -> HTTP, Job, Command
+    - Service -> HTTP
+    - Repository -> HTTP, Job, Command
+- Check for Complex data object:
+  There are some typical classes that should not contain too much business logic since their main purpose is to hold data. These classes are:
+  - Resources
+  - Requests
+  - DataTransferObjects (DTO)
+  - Value Objects
+  - Mail
+  - Notification
+  - Event
+  - Listener
+        
+  If a class that contains too much business logic, it needs to be warn. 'Too much' means that the cyclomatic complexity of the class is larger than 3.
+- Check for Principles
+  Does the code follow the Single Responsibility Principle (SRP) and Dont Repeat Yourself (DRY) principle. If not explain where the user needs to make changes.
+- Check for naming conventions
+  If a pull request code is violating the naming convention it needs to be warn.
+- Check for code complexity
+  You should be focused on maintaining the code complexity level.
+- Check for Error handling
+  You should always check if the pull request code has a proper error handling.
+  - Are all error scenarios covered in the code?
+  - Are the error messages clear and helpful?
+  - Is the code handling errors gracefully?
+- Check for Security
+  - Are sensitive data and credentials stored securely?
+  - Are all external libraries and packages up-to-date?
+  - Is the code protected against common security vulnerabilities such as SQL injection and cross-site scripting (XSS)?
         
 \`\`\`
 ${code}
@@ -82,11 +115,11 @@ ${code}
 
   ANSWER_TEMPLATE:
     description: 'The template for the answer sent to the GitHub comment.'
-    default: 'AI Code Review:
+    default: 'Raktbeej Code Review:
 
 ### Summary:
 
-${answer}'
+${answer}"
 runs:
   using: 'node16'
   main: 'dist/index.js'