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.

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

Fix post review API #371

Merged
merged 2 commits into from
Aug 22, 2021
Merged

Fix post review API #371

merged 2 commits into from
Aug 22, 2021

Conversation

jdaok
Copy link
Contributor

@jdaok jdaok commented Aug 18, 2021

This PR fixes a small global variable bug that caused the publish review endpoint to return an error when the review's "is_draft" was set to false.

@@ -416,6 +416,7 @@ def review_post_handler(user):
"""

def fetch_params():
global REVIEW_TEXT_MIN_LENGTH
Copy link
Member

Choose a reason for hiding this comment

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

This fix looks incorrect to me. We should not be reassigning a global variable (especially which we want to asssume to be constant) while processing a request as it will be reflected in future requests. For example, a draft review may set to REVIEW_TEXT_MIN_LENGTH, then another review comes which is not a draft, since REVIEW_TEXT_MIN_LENGTH is None the minimum length check will not work as expected.

@amCap1712
Copy link
Member

I have added a new fix and a test. This should hopefully fix the issue.

@amCap1712 amCap1712 merged commit 1260411 into metabrainz:master Aug 22, 2021
@github-actions
Copy link

Unit Test Results

    1 files  ±0      1 suites  ±0   47s ⏱️ ±0s
142 tests ±0  142 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 1260411. ± Comparison against base commit 1260411.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants