feat: add country-based data deletion for GDPR erasure (closes #42)#50
Open
Suraj80 wants to merge 2 commits intoguibranco:mainfrom
Open
feat: add country-based data deletion for GDPR erasure (closes #42)#50Suraj80 wants to merge 2 commits intoguibranco:mainfrom
Suraj80 wants to merge 2 commits intoguibranco:mainfrom
Conversation
Reviewer's GuideImplements GDPR-oriented visitor data erasure by country by wiring a new admin-post handler, DB utilities for country-based deletion and logging, and a UI panel on the visitors admin screen to trigger the operation and display feedback. Sequence diagram for GDPR delete-by-country admin workflowsequenceDiagram
actor Admin
participant VisitorsAdminScreen
participant Browser
participant WordPressAdminPost
participant IpQuery_Admin
participant IpQuery_DB
participant WPDB
participant WPDebugLog
Admin->>VisitorsAdminScreen: Open visitors.php
VisitorsAdminScreen->>IpQuery_DB: get_distinct_countries()
IpQuery_DB->>WPDB: SELECT DISTINCT country, country_code ...
WPDB-->>IpQuery_DB: Result rows
IpQuery_DB-->>VisitorsAdminScreen: Countries array
VisitorsAdminScreen-->>Admin: Render Delete by Country panel
Admin->>Browser: Select country and click Delete Records
Browser->>Browser: Show confirm dialog
Browser->>WordPressAdminPost: POST action=ipquery_delete_by_country
WordPressAdminPost->>IpQuery_Admin: handle_delete_by_country()
IpQuery_Admin->>IpQuery_Admin: check_admin_referer()
IpQuery_Admin->>IpQuery_Admin: current_user_can(manage_options)
IpQuery_Admin->>IpQuery_Admin: Sanitize and normalize country_code
IpQuery_Admin->>IpQuery_DB: delete_by_country(country_code)
IpQuery_DB->>WPDB: DELETE FROM table WHERE country_code = ?
WPDB-->>IpQuery_DB: rows_deleted or false
IpQuery_DB-->>IpQuery_Admin: rows_deleted or false
alt rows_deleted is not false
IpQuery_Admin->>IpQuery_DB: log_action(message)
IpQuery_DB->>WPDebugLog: error_log([IpQuery] message) (if WP_DEBUG_LOG)
end
IpQuery_Admin->>WordPressAdminPost: wp_safe_redirect(...country_deleted, country_code)
WordPressAdminPost-->>Browser: Redirect to visitors.php
Browser->>VisitorsAdminScreen: GET with country_deleted and country_code
VisitorsAdminScreen->>VisitorsAdminScreen: Render success notice
VisitorsAdminScreen-->>Admin: Show "%d record(s) deleted for country" notice
Class diagram for IpQuery_Admin and IpQuery_DB GDPR additionsclassDiagram
class IpQuery_Admin {
+init() void
+handle_settings_save() void
+enqueue_assets() void
+handle_delete_ip() void
+handle_delete_by_country() void
+handle_purge() void
+handle_manual_lookup() void
+ajax_chart_data() void
}
class IpQuery_DB {
+delete_ip(ip string) void
+delete_by_country(country_code string) "int|false"
+get_distinct_countries() array
+log_action(message string) void
}
IpQuery_Admin --> IpQuery_DB : uses delete_by_country
IpQuery_Admin --> IpQuery_DB : uses log_action
IpQuery_Admin --> IpQuery_DB : uses get_distinct_countries
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds country-based bulk deletion capability to the IP Query admin panel. A new admin form allows authorized users to select a country and permanently delete all associated visitor records, with a confirmation dialog, nonce validation, optional audit logging, and a success notice displaying the count deleted. ChangesCountry-Based Data Deletion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 25 minutes and 19 seconds.Comment |
|
Failed to generate code suggestions for PR |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
handle_delete_by_country(), the redirect always includescountry_deletedeven when$deleted === false, which makes a DB failure indistinguishable from a successful operation that deleted 0 rows; consider branching the redirect (or notice) to separately handle the error case. - The delete-by-country panel in
visitors.phpuses an inlinestyle="margin-top:24px;"; consider moving this into an existing CSS class or shared stylesheet to keep presentational concerns out of the view markup.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handle_delete_by_country()`, the redirect always includes `country_deleted` even when `$deleted === false`, which makes a DB failure indistinguishable from a successful operation that deleted 0 rows; consider branching the redirect (or notice) to separately handle the error case.
- The delete-by-country panel in `visitors.php` uses an inline `style="margin-top:24px;"`; consider moving this into an existing CSS class or shared stylesheet to keep presentational concerns out of the view markup.
## Individual Comments
### Comment 1
<location path="includes/class-ipquery-admin.php" line_range="225-231" />
<code_context>
+ *
+ * @return void
+ */
+ public static function handle_delete_by_country(): void {
+ check_admin_referer( 'ipquery_delete_by_country' );
+ if ( ! current_user_can( 'manage_options' ) ) {
+ wp_die( esc_html__( 'Not allowed.', 'ipquery' ) );
+ }
+
+ $country_code = strtoupper(
+ sanitize_text_field( wp_unslash( $_POST['country_code'] ?? '' ) )
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** Uppercasing the country code before deletion may prevent matching existing rows.
`country_code` is already sourced from a DB-backed `<select>`, so changing it to uppercase here assumes all stored values are uppercase. If any existing or future rows use lowercase/mixed-case codes (e.g. `us`), the delete will silently affect 0 rows. Either drop `strtoupper()` for the DB operation (only uppercase for display), or ensure `country_code` is consistently normalized at write-time throughout the codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+225
to
+231
| public static function handle_delete_by_country(): void { | ||
| check_admin_referer( 'ipquery_delete_by_country' ); | ||
| if ( ! current_user_can( 'manage_options' ) ) { | ||
| wp_die( esc_html__( 'Not allowed.', 'ipquery' ) ); | ||
| } | ||
|
|
||
| $country_code = strtoupper( |
There was a problem hiding this comment.
issue (bug_risk): Uppercasing the country code before deletion may prevent matching existing rows.
country_code is already sourced from a DB-backed <select>, so changing it to uppercase here assumes all stored values are uppercase. If any existing or future rows use lowercase/mixed-case codes (e.g. us), the delete will silently affect 0 rows. Either drop strtoupper() for the DB operation (only uppercase for display), or ensure country_code is consistently normalized at write-time throughout the codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/views/visitors.php`:
- Around line 30-34: Replace the hardcoded translatable string using "record(s)"
with a proper singular/plural localization via _n(); specifically, call _n( '1
record deleted for country: %s.', '%d records deleted for country: %s.', (int)
$_GET['country_deleted'], 'ipquery' ) (or similar) to choose the correct form
based on (int) $_GET['country_deleted'], then pass the resulting localized
string into printf/sprintf and substitute the count and the esc_html(
strtoupper( sanitize_text_field( wp_unslash( $_GET['country_code'] ?? '' ) ) )
); keep existing escaping and nonce-ignore comments and ensure the number
argument used for _n() is the same (int) $_GET['country_deleted'].
In `@includes/class-ipquery-admin.php`:
- Around line 254-259: The redirect currently casts $deleted to (int) which
converts a DB failure (false) into 0, losing the false-vs-0 distinction; update
the code that builds the 'country_deleted' query value (used in the
wp_safe_redirect/admin_url call in class-ipquery-admin.php) to preserve false by
explicitly checking $deleted === false and setting the param to a string like
'false' (otherwise cast to (int) or string for numeric counts), e.g. compute
$deleted_param = $deleted === false ? 'false' : (string) (int) $deleted and use
that instead of (int) $deleted so the UI can distinguish failure from zero rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0b9ab1c-300d-46a4-9dc1-24ba8130b56f
📒 Files selected for processing (3)
admin/views/visitors.phpincludes/class-ipquery-admin.phpincludes/class-ipquery-db.php
…e inline style to CSS
Author
|
Thanks for the detailed review! I've addressed all the feedback in the latest commit |
gstraccini
Bot
added
hacktoberfest
![gstraccini[bot]](https://avatars.githubusercontent.com/in/480132?s=60&v=4){kind=small}
gstraccini
Bot
added
the
🚦 awaiting triage
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
2 participants
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.
Closes #42
📑 Description
Adds country-based visitor data deletion to support GDPR right-to-erasure workflows.
Changes made across 3 files:
includes/class-ipquery-db.phpdelete_by_country(string $country_code): int|false— deletes all visitor rows matching a given ISO country codeget_distinct_countries(): array— returns distinct countries that have records, used to populate the dropdownlog_action(string $message): void— writes audit entries to the WordPress debug log whenWP_DEBUG_LOGis enabledincludes/class-ipquery-admin.phpadmin_post_ipquery_delete_by_countryhook ininit()handle_delete_by_country()— validates nonce, checks capability, callsIpQuery_DB::delete_by_country(), logs the action, and redirects with a success noticeadmin/views/visitors.phpcountry_deletedredirect paramrequiredattribute, and anonclickconfirmation dialogChecks
Does this introduce a breaking change?
Additional Information
IpQuery_DB::log_action()which only writes whenWP_DEBUG_LOGistrue, consistent with WordPress best practices. Note: existing handlers (handle_delete_ip,handle_purge) do not currently log — this PR introduces logging specifically to satisfy the acceptance criteria of issue [FEATURE] Add country-based data deletion support #42$deleted !== falsecheck distinguishes a real DB failure from a legitimate "0 rows deleted" resultSummary by Sourcery
Add support for deleting visitor records by country to help fulfill GDPR right-to-erasure requests.
New Features:
Enhancements:
Summary by CodeRabbit