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

Validate URLs as URLs in API #293

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

rgossiaux
Copy link
Contributor

Now it will complain if you don't enter a full URL with protocol,
matching the old UI behavior. Without this change it doesn't care and
just normalizes whatever you input.

Now it will complain if you don't enter a full URL with protocol,
matching the old UI behavior. Without this change it doesn't care and
just normalizes whatever you input.
@rgossiaux rgossiaux marked this pull request as draft January 2, 2021 10:56
@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #293 (b4c379e) into master (9fd927d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #293   +/-   ##
=======================================
  Coverage   77.10%   77.10%           
=======================================
  Files          99       99           
  Lines        1878     1878           
=======================================
  Hits         1448     1448           
  Misses        430      430           
Impacted Files Coverage Δ
api/serializers.py 68.29% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fd927d...b4c379e. Read the comment docs.

@rgossiaux
Copy link
Contributor Author

Actually i think this is too strict now (it rejects foo.com when the old UI accepts it for example, and I think it's a reasonable input) so I'm going to iterate on it more.

@rawxfish rawxfish temporarily deployed to smallboard-rgossiaux-ap-5yyasg January 2, 2021 10:57 Inactive
Copy link
Contributor

@erwa erwa left a comment

Choose a reason for hiding this comment

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

Looks good. It seems you marked this PR as a "Draft"? I think you have to remove that to merge this.

@asdfryan
Copy link
Collaborator

asdfryan commented Jan 8, 2021

I see this is still in draft -- is this ready for review?

@@ -96,7 +96,7 @@ class PuzzleSerializer(serializers.ModelSerializer):
tags = serializers.SerializerMethodField(required=False)
guesses = serializers.SerializerMethodField()
# Have to specify this explicitly for validate_url to run
url = serializers.CharField()
url = serializers.URLField()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this allow an empty url? I seem to recall we discussed at some point we might want to create a puzzle placeholder where the url was left blank (and perhaps filled in later).

@erwa
Copy link
Contributor

erwa commented Jan 9, 2021

Actually i think this is too strict now (it rejects foo.com when the old UI accepts it for example, and I think it's a reasonable input) so I'm going to iterate on it more.

I'm actually fine with requiring a scheme prefix, as in practice, we'll be copying and pasting URLs from the browser, and the http/https will automatically be included when copying. But if you want to support more user-friendly inputs like foo.com (which makes testing slightly easier), you could do something like auto-prepend http:// when it's missing. Then you can still use URLValidator. (If the URL really should be https://, one can always edit the URL after the puzzle is created.)

Here's a related SO post: https://stackoverflow.com/questions/49983328/cleanest-way-to-allow-empty-scheme-in-django-urlfield

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.

4 participants