8000 fix: don't allow setting an invalid rating by akhilnarang · Pull Request #22633 · frappe/frappe · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: don't allow setting an invalid rating #22633

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.

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

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

akhilnarang
Copy link
Member

Convert anything <0 to 0, and anything >1 to 1

Resolves #16245

Before this patch:

❯ curl -sH "Authorization: token $token" http://localhost:8000/api/resource/Item/SKU00001 -X PUT --json '{"rating": 3.14}' | jq -r '.data.rating'
3.14

After this patch:

❯ curl -sH "Authorization: token $token" http://localhost:8000/api/resource/Item/SKU00001 -X PUT --json '{"rating": 3.14}' | jq -r '.data.rating'
1

@akhilnarang akhilnarang requested review from a team and surajshetty3416 and removed request for a team October 5, 2023 09:44
@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label Oct 5, 2023
@codecov
Copy link
codecov bot commented Oct 5, 2023

Codecov Report

Merging #22633 (111a80f) into develop (eee418a) will decrease coverage by 0.01%.
Report is 25 commits behind head on develop.
The diff coverage is 14.28%.

❗ Current head 111a80f differs from pull request most recent head 4c93e60. Consider uploading reports for the commit 4c93e60 to get more accurate results

@@             Coverage Diff             @@
##           develop   #22633      +/-   ##
===========================================
- Coverage    62.34%   62.34%   -0.01%     
===========================================
  Files          768      768              
  Lines        73515    73519       +4     
  Branches      6332     6332              
===========================================
+ Hits         45834    45836       +2     
- Misses       24102    24104       +2     
  Partials      3579     3579              
Flag Coverage Δ
server 66.38% <14.28%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ankush
Copy link
Member
ankush commented Oct 10, 2023

@akhilnarang add one test case covering all 3 cases. Rest lgtm.

@ankush ankush changed the title feat: don't allow setting an invalid rating fix: don't allow setting an invalid rating Oct 10, 2023
@ankush ankush added the backport version-14-hotfix backport to version 14 label Oct 10, 2023
@@ -371,6 +371,13 @@ def get_valid_dict(
elif df.fieldtype in float_like_fields and not isinstance(value, float):
value = flt(value)

elif df.fieldtype == "Rating":
Copy link
Member

Choose a reason for hiding this comment

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

It's best to move this to document's _validate method. get_valid_dict is serializer function, so it shouldn't be applying business logic here.

Convert anything <0 to 0, and anything >1 to 1

Signed-off-by: Akhil Narang <me@akhilnarang.dev>
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@ankush ankush removed backport version-14-hotfix backport to version 14 add-test-cases Add test case to validate fix or enhancement labels Oct 10, 2023
@ankush ankush merged commit b601131 into frappe:develop Oct 10, 2023
@akhilnarang akhilnarang deleted the validate-rating branch October 12, 2023 08:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2023
@ankush ankush added the backport version-14-hotfix backport to version 14 label Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

limit rating field type values to max 1
2 participants
0