Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix the category update operation #356

Merged
merged 8 commits into from
Jun 27, 2023
Merged

Fix the category update operation #356

merged 8 commits into from
Jun 27, 2023

Conversation

phonglynosto
Copy link
Contributor

The category update operation does not work at the moment due to using wrong query and the result handler parse the response in a wrong way

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • I have assigned the correct milestone or created one if non-existent.
  • I have correctly labeled this pull request.
  • I have linked the corresponding issue in this description.
  • I have updated the corresponding Jira ticket.
  • I have requested a review from at least 2 reviewers
  • I have checked the base branch of this pull request
  • I have checked my code for any possible security vulnerabilities

@phonglynosto phonglynosto self-assigned this Jun 14, 2023
supercid
supercid requested changes Jun 14, 2023
View reviewed changes
Copy link
Member

@supercid supercid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR name is also not descriptive. One suggestion would be "Add category availability into category model"

src/Result/Graphql/Category/CategoryUpdateResultHandler.php Outdated Show resolved Hide resolved
src/Result/Graphql/Category/CategoryUpdateResultHandler.php Outdated Show resolved Hide resolved
@phonglynosto
Copy link
Contributor Author

phonglynosto commented Jun 14, 2023 •
edited
Loading

The PR name is also not descriptive. One suggestion would be "Add category availability into category model"

But actually we do not just add the availability into model, we also fix the update operation by fixing the query for request and the result handler.

supercid
supercid requested changes Jun 14, 2023
View reviewed changes
Copy link
Member

@supercid supercid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing updating the mock class and tests since you're changing the interface
For example, at tests/_support/MockCategory.php

@phonglynosto
Copy link
Contributor Author

Due to the fact that I also add the operation/CategoryUpdate, I think that I should add test for it too ?

@supercid
Copy link
Member

Yep, please add a test

@phonglynosto phonglynosto force-pushed the hotfix/fix-update-category branch 3 times, most recently from 4746b84 to b97461f Compare June 15, 2023 07:54
@phonglynosto phonglynosto force-pushed the hotfix/fix-update-category branch 2 times, most recently from f373078 to 1433269 Compare June 16, 2023 08:01
@phonglynosto
Fix the HttpTest
0addabc 
Currently in local we use debian distro and the github runner use
ubuntu so the curl error for connection refuse is difference so
I have change the assert for connection refuse test. Maybe problem
is curl version difference
@phonglynosto phonglynosto merged commit 8f14a91 into master Jun 27, 2023
@phonglynosto phonglynosto deleted the hotfix/fix-update-category branch June 27, 2023 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supercid supercid supercid approved these changes

Assignees

@phonglynosto phonglynosto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phonglynosto @supercid