feat(api): fixed patch upload by id endpoint #3042
Conversation
caa243d to
a86a70a
Compare
3cc8704 to
9e41455
Compare
|
@harshitg927 It looks like there are a lot of formatting changes in this PR, especially around tab/indentation length. These changes make the diff harder to review and aren't related to the actual functionality. Could you revert these formatting changes and keep the diff focused on the functional updates? |
|
Hello @its-sushant , I made the indentation changes because of the checks failing for codesniffer, if I revert back the changes the checks may fail. |
Yes @harshitg927 You can revert those changes for now! |
33a8db3 to
c9deda2
Compare
|
@its-sushant I have made the necessary formatting changes, PTAL. |
There was a problem hiding this comment.
@harshitg927 Please take a look into the suggested changes!
| $groupId, | ||
| $perm, | ||
| $this->dbHelper->getDbManager() | ||
| $groupId, | ||
| $perm, | ||
| $this->dbHelper->getDbManager() |
There was a problem hiding this comment.
Why do we need this indentation change here?
| if (!array_key_exists($assignee, $userList)) { | ||
| throw new HttpNotFoundException( | ||
| "New assignee does not have permission on upload."); | ||
| if (!array_key_exists($assignee, $userList)) { | ||
| throw new HttpNotFoundException( | ||
| "New assignee does not have permission on upload."); |
| array_key_exists(self::FILTER_STATUS, $bodyContent) && | ||
| in_array(strtolower($bodyContent[self::FILTER_STATUS]), self::VALID_STATUS) | ||
| ) { | ||
| $newStatus = strtolower($bodyContent[self::FILTER_STATUS]); |
There was a problem hiding this comment.
| array_key_exists(self::FILTER_STATUS, $bodyContent) && | |
| in_array(strtolower($bodyContent[self::FILTER_STATUS]), self::VALID_STATUS) | |
| ) { | |
| $newStatus = strtolower($bodyContent[self::FILTER_STATUS]); | |
| array_key_exists(self::FILTER_STATUS, $bodyContent) && | |
| in_array(strtolower($bodyContent[self::FILTER_STATUS]), self::VALID_STATUS) | |
| ) { | |
| $newStatus = strtolower($bodyContent[self::FILTER_STATUS]); |
| $uploadBrowseProxy->setStatusAndComment($id, $status, $comment); | ||
| $uploadBrowseProxy->setStatusAndComment($id, $status, $comment); |
| array_key_exists("uploadDescription", $bodyContent) && | ||
| strlen(trim($bodyContent["uploadDescription"])) > 0 | ||
| ) { | ||
| ) { |
There was a problem hiding this comment.
| ) { | |
| ) { |
| * @test | ||
| * -# Test for UploadController::updateUpload() | ||
| * -# Check if response status is 202 | ||
| */ | ||
| public function testUpdateUpload() | ||
| { | ||
| * @test | ||
| * -# Test for UploadController::updateUpload() | ||
| * -# Check if response status is 202 | ||
| */ | ||
| public function testUpdateUpload() | ||
| { |
There was a problem hiding this comment.
Please fix the indentation. There are lot of indentation changes here which is not required in this function. Please take a look!
|
Hello @harshitg927. Any update on this. |
|
@its-sushant Hello, sorry for the delay, there were some issues with my computer, I will fix the linting issues and push the code ASAP. |
c00b274 to
3c9b4e4
Compare
| @@ -715,22 +709,21 @@ public function updateUpload($request, $response, $args) | |||
| } | |||
| $uploadBrowseProxy->setStatusAndComment($id, $status, $comment); | |||
| } | |||
| // Handle update of name | |||
There was a problem hiding this comment.
Is there a reason behind removing the comment: // Handle update of name?
There was a problem hiding this comment.
I have included the comments which were previously there along with some comments for the work done in this pull request.
| $comment = $bodyContent; | ||
| } | ||
| } | ||
| $newStatus = strtolower($bodyContent[self::FILTER_STATUS]); |
There was a problem hiding this comment.
Logic for validating a comment for specific status: Closed or Rejected was mandatory. We should keep that, There has to be a specific comment provided for these two specific Status. As to why rejected or closed
| $userList = $userDao->getUserChoices($groupId); | ||
| if (!array_key_exists($assignee, $userList)) { | ||
| throw new HttpNotFoundException( | ||
| "New assignee does not have permission on upload."); | ||
| } | ||
| $uploadBrowseProxy->updateTable("assignee", $id, $assignee); | ||
| } | ||
| // Handle new status | ||
|
|
There was a problem hiding this comment.
Also type checking if comment is actual string would make more sense.
| @@ -715,22 +709,21 @@ public function updateUpload($request, $response, $args) | |||
| } | |||
| $uploadBrowseProxy->setStatusAndComment($id, $status, $comment); | |||
There was a problem hiding this comment.
What if the user didnt provide the comment in request body? There is no initialization of the comment?
There was a problem hiding this comment.
Done, I have initialized the comment field with empty string.
|
@harshitg927 , Any updates on this? |
Signed-off-by: Harshit Gandhi <gandhiharshit716@gmail.com>
|
@Kaushl2208 , I have made the changes requested, PTAL. |
There was a problem hiding this comment.
Hey @harshitg927 , Most of the things look good. There are few edge cases that I was trying:
- There should be an exception handling: Invalid Status Type right???
As for Controller Test cases:
- Missing or empty comment for Closed/Rejected
- Comment not a string
- Invalid status
- Name/description missing or empty
These edge cases will make good reliable coverage.
| comment: | ||
| type: string | ||
| description: The comment for new status | ||
| description: Optional comment for the status change |
There was a problem hiding this comment.
Umm something Like this makes more sense right?
Required when status is 'Closed' or 'Rejected'.
Description
This PR modifies the updateUpload method in UploadController to properly handle all parameters from the request body instead of query parameters. This allows users to update upload status, assignee, name, and description via a single JSON payload, making the API more RESTful and consistent with the Swagger documentation.
Changes
How to test
{
"status": "Closed",
"assignee": 3,
"comment": "Upload reviewed and closed",
"name": "new_upload_name.zip",
"uploadDescription": "Updated description"
}
Fixes: issue#3041