clean(ZMS-2872): added scope and department to delete message #711
Conversation
WalkthroughThe changes involve updates to success messages in the Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
zmsadmin/src/Zmsadmin/ScopeDelete.php (1)
31-42: Consider optimizing the data fetching patternThe current implementation makes an additional API call to fetch the scope name. Consider these alternatives:
- Pass the scope name from the UI/previous page as a query parameter
- Fetch this data earlier in the flow and cache it
- Batch the data fetching with other required operations
This would reduce API calls and improve performance, especially if this pattern is replicated across other delete controllers.
zmsadmin/src/Zmsadmin/DepartmentDelete.php (1)
32-34: Remove debug code and commented out linesDebug statements and commented out code should not be committed to the codebase.
Apply this diff to clean up the code:
- // xdebug_var_dump($entity); - - // $departmentName = $entity->offsetGet('contact')->offsetGet('name');zmsadmin/src/Zmsadmin/OwnerOverview.php (1)
Line range hint
1-39: Consider separating deletion feedback handlingThe
OwnerOverviewclass is now handling both the overview display and deletion feedback responsibilities. Consider extracting the deletion feedback handling into a separate trait or service class to improve separation of concerns and maintainability.This would:
- Make the code more modular and reusable
- Simplify testing
- Make it easier to modify the feedback mechanism in the future
Would you like assistance in designing this separation?
zmsadmin/custom_templates/element/helper/messages.twig (2)
107-107: LGTM! Consider extending the pattern to related messages.The addition of
departmentNameimproves user feedback. For consistency, consider adding the department name to the related messages indepartment_createdanddepartment_savedblocks as well.{% block department_created %} {{ block('success_start') }} - Die Behörde wurde {{ timeString }} erfolgreich angelegt. + Die Behörde {{ departmentName }} wurde {{ timeString }} erfolgreich angelegt. {{ block('success_end') }} {% endblock %} {% block department_saved %} {{ block('success_start') }} - Die Behörde wurde {{ timeString }} erfolgreich aktualisiert. + Die Behörde {{ departmentName }} wurde {{ timeString }} erfolgreich aktualisiert. {{ block('success_end') }} {% endblock %}
170-170: LGTM! Consider extending the pattern to related messages.The addition of
scopeNameimproves user feedback. For consistency, consider adding the scope name to the related messages inscope_createdandscope_savedblocks as well.{% block scope_created %} {% set successTitle = "Anlegen erfolgreich" %} {{ block('success_start') }} - Der Standort wurde {{ timeString }} erfolgreich angelegt. + Der Standort {{ scopeName }} wurde {{ timeString }} erfolgreich angelegt. {{ block('success_end') }} {% endblock %} {% block scope_saved %} {% set successTitle = "Speichern erfolgreich" %} {{ block('success_start') }} - Der Standort wurde {{ timeString }} erfolgreich aktualisiert. + Der Standort {{ scopeName }} wurde {{ timeString }} erfolgreich aktualisiert. {{ block('success_end') }} {% endblock %}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
zmsadmin/custom_templates/block/element/helper/messages.twig(2 hunks)zmsadmin/custom_templates/element/helper/messages.twig(2 hunks)zmsadmin/src/Zmsadmin/DepartmentDelete.php(1 hunks)zmsadmin/src/Zmsadmin/OwnerOverview.php(2 hunks)zmsadmin/src/Zmsadmin/ScopeDelete.php(1 hunks)
🔇 Additional comments (4)
zmsadmin/src/Zmsadmin/ScopeDelete.php (1)
41-42: Verify template usage and controller consistency
The redirect parameters look correct, but let's ensure:
- The template uses these parameters correctly
- Similar changes exist in other delete controllers for consistency
✅ Verification successful
The verification results show that:
-
The template usage is confirmed:
- The
scope_deletedblock exists in multiple message template files - The custom templates (
zmsadmin/custom_templates/*/messages.twig) use thescopeNameparameter in the message:Der Standort {{ scopeName }} wurde {{ timeString }} erfolgreich entfernt. - The default templates also have the
scope_deletedblock but without using thescopeNameparameter
- The
-
The controller consistency check shows multiple delete controllers exist, so let's verify if they follow similar patterns for success messages.
Redirect parameters are consistent with similar delete controllers
The verification shows that:
- The template correctly uses the
scope_deletedmessage block andscopeNameparameter in custom templates - Other delete controllers follow a similar pattern:
- All use a
*_deletedsuccess message key - Some controllers (like
DepartmentDelete.php) also pass entity names as parameters (departmentName)
- All use a
The changes are consistent with the codebase patterns and the templates are properly configured to handle these parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the template uses these parameters and verify consistency across controllers
# Check template usage
echo "Checking template usage..."
rg -A 5 "scope_deleted|scopeName" --type=twig
# Check other delete controllers for consistency
echo "Checking other delete controllers..."
fd -e php . | xargs rg -l "class \w+Delete extends BaseController"
Length of output: 10259
Script:
#!/bin/bash
# Check the redirect parameters in other delete controllers
echo "Checking redirect parameters in delete controllers..."
rg -B 2 -A 2 "'success' =>" zmsadmin/src/Zmsadmin/*Delete.php
Length of output: 2924
zmsadmin/src/Zmsadmin/DepartmentDelete.php (1)
42-43: LGTM! The success message now includes the department name
The modification improves user feedback by including the department name in the success message, which aligns with the PR objectives.
zmsadmin/custom_templates/block/element/helper/messages.twig (2)
107-107: LGTM! Enhanced feedback by including department name
The message now provides more specific feedback by including the department name, which improves user experience.
170-170: LGTM! Enhanced feedback by including scope name
The message now provides more specific feedback by including the scope name, which improves user experience.
Let's verify that both variables are properly passed from the controllers:
✅ Verification successful
Variables are properly passed and handled in the codebase
The verification shows that both variables are correctly handled:
- In
ScopeDelete.php, thescopeNameis properly extracted from the entity's contact name and passed to the redirect - In
OwnerOverview.php, thescopeNameis correctly received as a request parameter and passed to the template - The variable usage in the template matches the data flow in the controllers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that departmentName and scopeName variables are properly set in the controllers
# Check for departmentName in DepartmentDelete controller
rg -A 5 "departmentName.*=.*" --type php
# Check for scopeName in ScopeDelete controller
rg -A 5 "scopeName.*=.*" --type php
Length of output: 4616
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
zmsadmin/tests/Zmsadmin/ScopeDeleteTest.php (2)
23-27: Consider using a dedicated fixture for the GET request.While using the same fixture works functionally, it would be more accurate to use a separate fixture that specifically represents the GET response format. This would better reflect the real-world scenario where DELETE and GET endpoints might return different response structures.
Line range hint
14-34: Consider adding test cases for error scenarios.The current test only covers the successful deletion path. Consider adding test cases for:
- Invalid scope ID
- Permission denied scenarios
- API error responses
Example structure:
public function testInvalidScopeId() { // Test with non-existent scope ID } public function testApiError() { // Test handling of API error response }zmsadmin/tests/Zmsadmin/DepartmentDeleteTest.php (1)
33-33: Consider using a more specific assertion.While
assertStringContainsStringworks, consider using a more robust assertion that specifically checks the success parameter.-$this->assertStringContainsString('/owner/?success=department_deleted', $response->getHeaderLine('Location')); +$locationHeader = $response->getHeaderLine('Location'); +$this->assertMatchesRegularExpression('/^\/owner\/\?success=department_deleted$/', $locationHeader);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
zmsadmin/tests/Zmsadmin/DepartmentDeleteTest.php(1 hunks)zmsadmin/tests/Zmsadmin/ScopeDeleteTest.php(1 hunks)
🔇 Additional comments (2)
zmsadmin/tests/Zmsadmin/ScopeDeleteTest.php (1)
32-32: Verify success message format consistency across delete operations.
The success message format should be consistent across all delete operations (scope, department, etc.).
Let's verify the consistency:
✅ Verification successful
Success message format is consistent across delete operations
The success message format follows a consistent pattern across all delete operations:
- Scope:
/owner/?success=scope_deleted - Department:
/owner/?success=department_deleted - Organisation:
/owner/?success=organisation_deleted - Cluster:
/owner/?success=cluster_deleted
All operations redirect to the owner page with a success parameter following the pattern {entity}_deleted. The change from assertRedirect to assertStringContainsString is an implementation detail that doesn't affect the consistency of the success message format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check success message format consistency across delete operation tests
# Search for similar assertions in other delete operation tests
rg -A 1 "success=" "tests/" | grep -E "assertString|assertRedirect"
# Check for all success message definitions in templates
rg "success=" "templates/"
Length of output: 188
Script:
#!/bin/bash
# Let's first find the test files and template files in the repository
fd -t f "Test\.php$"
# Then search for success message patterns in all PHP files
rg -A 1 "success=" ".*\.php$"
# Also check for any redirect assertions
rg -A 1 "assert.*Redirect|assertString.*Contains" ".*\.php$"
Length of output: 21664
Script:
#!/bin/bash
# Let's check the specific test files for delete operations and their assertions
rg -A 1 "success=" "zmsadmin/tests/Zmsadmin/ScopeDeleteTest.php" "zmsadmin/tests/Zmsadmin/DepartmentDeleteTest.php" "zmsadmin/tests/Zmsadmin/OrganisationDeleteTest.php" "zmsadmin/tests/Zmsadmin/ClusterDeleteTest.php"
# Also check for similar assertion patterns in these files
rg -A 1 "assert.*Redirect|assertString.*Contains" "zmsadmin/tests/Zmsadmin/ScopeDeleteTest.php" "zmsadmin/tests/Zmsadmin/DepartmentDeleteTest.php" "zmsadmin/tests/Zmsadmin/OrganisationDeleteTest.php" "zmsadmin/tests/Zmsadmin/ClusterDeleteTest.php"
Length of output: 2508
zmsadmin/tests/Zmsadmin/DepartmentDeleteTest.php (1)
23-28: LGTM! Verify if the additional API call is necessary.
The additional readGetResult API call is added to support retrieving the department name for the success message. The implementation looks correct.
Let's verify if this additional API call is actually needed by checking the implementation:
✅ Verification successful
Let me gather more information to verify if the department name is actually used in the implementation.
The additional API call is necessary and correctly implemented
The verification confirms that the additional API call is required because:
- The custom message template uses
{{ departmentName }}to display the department's name in the deletion success message - The
DepartmentDeleteclass extracts the department name ($departmentName = $entity->name) from the GET response before deletion - This name is then passed to the success message parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if DepartmentDelete.php uses the department name from an additional API call
# Search for the readResponse method implementation in DepartmentDelete
ast-grep --pattern 'function readResponse($_) {
$$$
}'
# Search for any department name usage in success messages
rg -A 3 'department_deleted'
Length of output: 2800
Script:
#!/bin/bash
# Check the DepartmentDelete class implementation
ast-grep --pattern 'class DepartmentDelete {
$$$
}'
# Find where departmentName is used in the class
rg -A 3 '\$departmentName' zmsadmin/src/Zmsadmin/DepartmentDelete.php
Length of output: 486
Summary by CodeRabbit
New Features
Bug Fixes
Tests