Feat/downloadable permission rest #3092
Open
+65
−48
Conversation
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
Introduces two new REST API endpoints to grant and revoke downloadable product access for orders. Implements the corresponding controller methods to handle permission management, including input validation and response formatting.
Contributor
📝 WalkthroughWalkthroughAdds a .vscode ignore entry and extends order download permission handling: new optional REST parameters ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as REST Client
participant Controller as OrderControllerV2
participant Store as WC_Data_Store
participant Download as WC_Customer_Download
Client->>Controller: POST grant_order_downloads(order_id, product_id, download_remaining?, access_expires?)
Controller->>Store: Lookup existing permission for order/product/file
alt permission exists
Store-->>Controller: permission_id
Controller->>Download: Load existing permission (permission_id)
alt update params provided
Controller->>Download: set downloads_remaining / set access_expires
Download->>Store: save updated permission
Store-->>Controller: updated_id
end
else permission not found
Controller->>Download: create new WC_Customer_Download with file/order/product
alt params provided
Controller->>Download: set downloads_remaining / set access_expires
end
Download->>Store: insert new permission
Store-->>Controller: new_permission_id
end
Controller->>Controller: increment file counter / assemble response
Controller-->>Client: return grant result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@includes/REST/ProductController.php`:
- Around line 1902-1919: The revoke_access_to_download method currently deletes
customer download rows blindly by permission_id; update
revoke_access_to_download to first load the customer-download record (via
WC_Data_Store::load('customer-download') and the record lookup for the given
permission_id), verify it exists and that its download_id, product_id and
order_id match the request parameters, and confirm the current user/vendor is
authorized to revoke that specific permission (e.g., matches vendor/store
ownership or has capability); if not found or not owned, return a WP_Error with
a 404/403 status instead of proceeding to delete, and only call delete_by_id and
do_action('woocommerce_ajax_revoke_access_to_product_download', ...) after these
checks.
- Around line 1860-1866: The current truthy checks prevent setting downloads
remaining to 0 or explicitly clearing expiry; modify the conditional tests
around $remaining and $expiry in the ProductController download update block so
they check for null (e.g. $remaining !== null and $expiry !== null) before
calling $download->set_downloads_remaining(...) and
$download->set_access_expires(...), ensuring zero values and explicit expiry
updates are applied while leaving other values untouched.
- Around line 1811-1834: The grant_downloadable_access handler must first ensure
the order returned by dokan()->order->get($order_id) is a valid object before
calling $order->get_billing_email(), and must verify ownership so a vendor
cannot grant access for other vendors' orders/products. Fix by: validate $order
is not null/false and return a proper REST error if missing; fetch the current
vendor ID (e.g. dokan_get_current_user_id()) and check that the order (or
sub‑order) belongs to that vendor and that each $product (from
$product->get_id() / $product post author) belongs to the same vendor before
proceeding to build $granted_files; if any ownership check fails, skip that
product or return a permission error. Implement these checks in
grant_downloadable_access around the existing dokan()->order->get(...) and
product loop.
- Around line 323-351: The REST arg definition for access_expires is using an
unsupported type 'date'; update the 'access_expires' entry in the register_route
args (the array passed to register_rest_route in ProductController) to use
'type' => 'string' and add 'format' => 'date-time' (and optionally keep
'required' => false) so it conforms to WP REST API valid types and date-time
formatting rules; locate the 'access_expires' key near the
grant-downloadable-access route callback (grant_downloadable_access) and replace
the 'date' type accordingly.
includes/REST/ProductController.php
Outdated
Comment on lines
323
to
351
| $this->namespace, '/' . $this->base . '/grant-downloadable-access', [ | ||
| [ | ||
| 'methods' => WP_REST_Server::CREATABLE, | ||
| 'callback' => [ $this, 'grant_downloadable_access' ], | ||
| 'permission_callback' => [ $this, 'get_product_permissions_check' ], | ||
| 'args' => [ | ||
| 'order_id' => [ | ||
| 'description' => __( 'Order ID', 'dokan-lite' ), | ||
| 'type' => 'integer', | ||
| 'required' => true, | ||
| ], | ||
| 'product_ids' => [ | ||
| 'description' => __( 'Product IDs', 'dokan-lite' ), | ||
| 'type' => 'array', | ||
| 'items' => [ | ||
| 'type' => 'integer', | ||
| ], | ||
| 'required' => true, | ||
| ], | ||
| 'download_remaining' => [ | ||
| 'description' => __( 'Download Remaining', 'dokan-lite' ), | ||
| 'type' => 'integer', | ||
| 'required' => false, | ||
| ], | ||
| 'access_expires' => [ | ||
| 'description' => __( 'Access Expires', 'dokan-lite' ), | ||
| 'type' => 'date', | ||
| 'required' => false, | ||
| ], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WordPress REST API register_rest_route argument schema type date datetime supported values
💡 Result:
In register_rest_route( ... 'args' => ... ), date/datetime are not valid JSON Schema type values in WordPress. For a date/time argument you should use:
'my_date' => [
'type' => 'string',
'format' => 'date-time', // RFC3339
]Supported type values (WP REST arg schema) are: array, object, string, number, integer, boolean, null. [3]
Supported string format values in WordPress are: date-time (RFC3339), uri, email, ip, uuid, hex-color. [1]
🏁 Script executed:
# First, let's locate and examine the file
find . -name "ProductController.php" -type fRepository: getdokan/dokan
Length of output: 95
🏁 Script executed:
# Read the ProductController.php file around lines 323-351
cat -n includes/REST/ProductController.php | sed -n '320,355p'Repository: getdokan/dokan
Length of output: 1980
Fix REST argument type for access_expires.
Line 349 uses 'type' => 'date', which is not a supported WordPress REST API argument type. Use 'type' => 'string' with 'format' => 'date-time' instead. Valid REST arg types are: array, object, string, number, integer, boolean, null. The current type will cause REST schema validation failures.
♻️ Proposed fix
'access_expires' => [
'description' => __( 'Access Expires', 'dokan-lite' ),
- 'type' => 'date',
+ 'type' => 'string',
+ 'format' => 'date-time',
'required' => false,
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $this->namespace, '/' . $this->base . '/grant-downloadable-access', [ | |
| [ | |
| 'methods' => WP_REST_Server::CREATABLE, | |
| 'callback' => [ $this, 'grant_downloadable_access' ], | |
| 'permission_callback' => [ $this, 'get_product_permissions_check' ], | |
| 'args' => [ | |
| 'order_id' => [ | |
| 'description' => __( 'Order ID', 'dokan-lite' ), | |
| 'type' => 'integer', | |
| 'required' => true, | |
| ], | |
| 'product_ids' => [ | |
| 'description' => __( 'Product IDs', 'dokan-lite' ), | |
| 'type' => 'array', | |
| 'items' => [ | |
| 'type' => 'integer', | |
| ], | |
| 'required' => true, | |
| ], | |
| 'download_remaining' => [ | |
| 'description' => __( 'Download Remaining', 'dokan-lite' ), | |
| 'type' => 'integer', | |
| 'required' => false, | |
| ], | |
| 'access_expires' => [ | |
| 'description' => __( 'Access Expires', 'dokan-lite' ), | |
| 'type' => 'date', | |
| 'required' => false, | |
| ], | |
| $this->namespace, '/' . $this->base . '/grant-downloadable-access', [ | |
| [ | |
| 'methods' => WP_REST_Server::CREATABLE, | |
| 'callback' => [ $this, 'grant_downloadable_access' ], | |
| 'permission_callback' => [ $this, 'get_product_permissions_check' ], | |
| 'args' => [ | |
| 'order_id' => [ | |
| 'description' => __( 'Order ID', 'dokan-lite' ), | |
| 'type' => 'integer', | |
| 'required' => true, | |
| ], | |
| 'product_ids' => [ | |
| 'description' => __( 'Product IDs', 'dokan-lite' ), | |
| 'type' => 'array', | |
| 'items' => [ | |
| 'type' => 'integer', | |
| ], | |
| 'required' => true, | |
| ], | |
| 'download_remaining' => [ | |
| 'description' => __( 'Download Remaining', 'dokan-lite' ), | |
| 'type' => 'integer', | |
| 'required' => false, | |
| ], | |
| 'access_expires' => [ | |
| 'description' => __( 'Access Expires', 'dokan-lite' ), | |
| 'type' => 'string', | |
| 'format' => 'date-time', | |
| 'required' => false, | |
| ], |
🤖 Prompt for AI Agents
In `@includes/REST/ProductController.php` around lines 323 - 351, The REST arg
definition for access_expires is using an unsupported type 'date'; update the
'access_expires' entry in the register_route args (the array passed to
register_rest_route in ProductController) to use 'type' => 'string' and add
'format' => 'date-time' (and optionally keep 'required' => false) so it conforms
to WP REST API valid types and date-time formatting rules; locate the
'access_expires' key near the grant-downloadable-access route callback
(grant_downloadable_access) and replace the 'date' type accordingly.
includes/REST/ProductController.php
Outdated
Comment on lines
1811
to
1834
| public function grant_downloadable_access( $request ) { | ||
| $order_id = $request->get_param( 'order_id' ); | ||
| $product_ids = $request->get_param( 'product_ids' ); | ||
| $remaining = $request->get_param( 'download_remaining' ); | ||
| $expiry = $request->get_param( 'access_expires' ); | ||
|
|
||
| if ( ! is_array( $product_ids ) ) { | ||
| $product_ids = [ $product_ids ]; | ||
| } | ||
|
|
||
| $order = dokan()->order->get( $order_id ); | ||
| $granted_files = []; | ||
|
|
||
| foreach ( $product_ids as $product_id ) { | ||
| $product = wc_get_product( $product_id ); | ||
| if ( ! $product ) { | ||
| continue; | ||
| } | ||
| $files = $product->get_downloads(); | ||
|
|
||
| if ( ! $order->get_billing_email() ) { | ||
| continue; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Validate order existence and vendor/order ownership before granting access.
dokan()->order->get() can return null/invalid; calling $order->get_billing_email() would fatal. Also, without verifying that the order (or sub‑order) and product belong to the current vendor, a seller could grant access for other vendors’ orders/products.
🐛 Minimal guard for missing/invalid order
- $order = dokan()->order->get( $order_id );
+ $order = dokan()->order->get( $order_id );
+ if ( ! $order || ! $order->get_id() ) {
+ return new WP_Error(
+ 'dokan_rest_invalid_order',
+ __( 'Invalid order ID.', 'dokan-lite' ),
+ [ 'status' => 404 ]
+ );
+ }Please also add ownership checks (e.g., ensure the order/sub‑order and each product belong to the current vendor before granting).
🤖 Prompt for AI Agents
In `@includes/REST/ProductController.php` around lines 1811 - 1834, The
grant_downloadable_access handler must first ensure the order returned by
dokan()->order->get($order_id) is a valid object before calling
$order->get_billing_email(), and must verify ownership so a vendor cannot grant
access for other vendors' orders/products. Fix by: validate $order is not
null/false and return a proper REST error if missing; fetch the current vendor
ID (e.g. dokan_get_current_user_id()) and check that the order (or sub‑order)
belongs to that vendor and that each $product (from $product->get_id() /
$product post author) belongs to the same vendor before proceeding to build
$granted_files; if any ownership check fails, skip that product or return a
permission error. Implement these checks in grant_downloadable_access around the
existing dokan()->order->get(...) and product loop.
includes/REST/ProductController.php
Outdated
Comment on lines
1860
to
1866
| if ( $download ) { | ||
| if ( $remaining ) { | ||
| $download->set_downloads_remaining( $remaining ); | ||
| } | ||
| if ( $expiry ) { | ||
| $download->set_access_expires( $expiry ); | ||
| } |
Contributor
There was a problem hiding this comment.
Allow download_remaining = 0 (and explicit expiry updates).
Using truthy checks prevents setting zero downloads remaining or explicitly clearing/updating expiry. Use null checks instead.
🐛 Proposed fix
- if ( $remaining ) {
+ if ( null !== $remaining ) {
$download->set_downloads_remaining( $remaining );
}
- if ( $expiry ) {
+ if ( null !== $expiry && '' !== $expiry ) {
$download->set_access_expires( $expiry );
}🤖 Prompt for AI Agents
In `@includes/REST/ProductController.php` around lines 1860 - 1866, The current
truthy checks prevent setting downloads remaining to 0 or explicitly clearing
expiry; modify the conditional tests around $remaining and $expiry in the
ProductController download update block so they check for null (e.g. $remaining
!== null and $expiry !== null) before calling
$download->set_downloads_remaining(...) and $download->set_access_expires(...),
ensuring zero values and explicit expiry updates are applied while leaving other
values untouched.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@includes/REST/OrderControllerV2.php`:
- Around line 244-245: The code is skipping updates to download_remaining and
access_expires when values are zero or when an existing permission branch
executes; change the logic to check for presence of params (use isset or a
param-exists helper rather than truthy checks) for $remaining and $expiry, apply
those values to the permission object in both the "create" and "existing
permission" branches, and ensure you call the same save/update method after
applying them so explicit 0/empty values clear or update the stored fields;
reference the variables $remaining, $expiry and the permission create/update
blocks to locate where to apply the changes.
🧹 Nitpick comments (1)
includes/REST/OrderControllerV2.php (1)
63-72: Add validation fordownload_remainingandaccess_expiresparameters at the REST layer.Both fields are persisted to the database without validation:
download_remaininglacks bounds checking and could accept negative values, whileaccess_expiresaccepts any string without verifying date format. Add sanitize/validate callbacks in the REST schema (consistent with other parameters in this controller) to ensure data integrity beforeWC_Customer_Download::save()is called.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@includes/REST/OrderControllerV2.php`:
- Around line 160-168: The bug is that the loop in OrderControllerV2 filtering
downloads checks an undefined $products_by_id, causing all items to be skipped;
update the condition to check the existing lookup map $products (i.e., replace
isset($products_by_id[$product_id]) with isset($products[$product_id])) and keep
the subsequent assignment $download->product = $products[$product_id] so
downloads are correctly collected and prepared for response in the same method.
🧹 Nitpick comments (1)
includes/REST/OrderControllerV2.php (1)
260-269: Redundant condition:$inserted_idis guaranteed truthy here.The check at line 260 is unnecessary since lines 245-247 already
continuewhen$inserted_idis falsy. Consider removing this nesting to reduce indentation.♻️ Proposed simplification
$download->save(); } - if ( $inserted_id ) { - ++$file_counter; - if ( $file->get_name() ) { - $file_count = $file->get_name(); - } else { - /* translators: numeric number of files */ - $file_count = sprintf( __( 'File %d', 'dokan-lite' ), $file_counter ); - } - $data[ $inserted_id ] = $file_count; + ++$file_counter; + if ( $file->get_name() ) { + $file_count = $file->get_name(); + } else { + /* translators: numeric number of files */ + $file_count = sprintf( __( 'File %d', 'dokan-lite' ), $file_counter ); } + $data[ $inserted_id ] = $file_count; } }
Comment on lines
+160
to
+168
| // Filter downloads with existing products and prepare response. | ||
| $downloads = []; | ||
| foreach ( $download_permissions as $download ) { | ||
| $product_id = intval( $download->product_id ); | ||
| if ( isset( $products_by_id[ $product_id ] ) ) { | ||
| $download->product = $products[ $product_id ]; | ||
| $downloads[] = $this->prepare_data_for_response( $download, $request ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Critical bug: Undefined variable $products_by_id causes empty results.
Line 164 references $products_by_id but the lookup map is stored in $products (line 151-157). This condition will always evaluate to false since $products_by_id is undefined, resulting in an empty $downloads array.
🐛 Proposed fix
// Filter downloads with existing products and prepare response.
$downloads = [];
foreach ( $download_permissions as $download ) {
$product_id = intval( $download->product_id );
- if ( isset( $products_by_id[ $product_id ] ) ) {
+ if ( isset( $products[ $product_id ] ) ) {
$download->product = $products[ $product_id ];
$downloads[] = $this->prepare_data_for_response( $download, $request );
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Filter downloads with existing products and prepare response. | |
| $downloads = []; | |
| foreach ( $download_permissions as $download ) { | |
| $product_id = intval( $download->product_id ); | |
| if ( isset( $products_by_id[ $product_id ] ) ) { | |
| $download->product = $products[ $product_id ]; | |
| $downloads[] = $this->prepare_data_for_response( $download, $request ); | |
| } | |
| } | |
| // Filter downloads with existing products and prepare response. | |
| $downloads = []; | |
| foreach ( $download_permissions as $download ) { | |
| $product_id = intval( $download->product_id ); | |
| if ( isset( $products[ $product_id ] ) ) { | |
| $download->product = $products[ $product_id ]; | |
| $downloads[] = $this->prepare_data_for_response( $download, $request ); | |
| } | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
164-164: Avoid unused local variables such as '$products_by_id'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@includes/REST/OrderControllerV2.php` around lines 160 - 168, The bug is that
the loop in OrderControllerV2 filtering downloads checks an undefined
$products_by_id, causing all items to be skipped; update the condition to check
the existing lookup map $products (i.e., replace
isset($products_by_id[$product_id]) with isset($products[$product_id])) and keep
the subsequent assignment $download->product = $products[$product_id] so
downloads are correctly collected and prepared for response in the same method.
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.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.