Add rest-auth helpers #125

jayvdb · 2020-07-14T11:11:01Z

Closes #113

jayvdb · 2020-07-14T11:16:31Z

Due to the high usage of rest-auth, it is likely others will benefit from this.

I dont have time to build extensive tests for this right now, covering all of the branches, as the drf-gis PR is the main one I want to be working on. Feel free to close if you need 100% coverage, and dont want this open for a while.

~~The part which is easiest to test, and IMO most important, is the JWT login. Just building tests for that is something I'd be happy to get done if that will be sufficient to get this merged.~~

jayvdb · 2020-07-14T11:20:52Z

drf_spectacular/contrib/rest_auth.py

+
+
+class DRFDefaultDetailSerializer(serializers.Serializer):
+    detail = serializers.CharField(read_only=True, required=False)


This is one of the responses for #101

Note that is would be highly desirable to fix #101 first! This is a major bug in the current schemas emitted by drf-spectacular.

Doing that first will cement in the serializer name, and thus the component name, which will be in the schema, and then it can be used there and elsewhere.

i'm currently trying out some ideas for #101 but it is a bit tricker that one would expect.

@tfranzel , please comment here. This is the most important part, and you ignored it.

i have not ignored it. my time is also limited and i wanted to give a well thought out review. will comment on it very soon.

drf_spectacular/contrib/rest_auth.py

jayvdb · 2020-07-14T13:24:26Z

Also worth noting, this doesnt include the social auth views provided by rest-auth.

codecov · 2020-07-14T15:30:07Z

Codecov Report

Merging #125 into master will decrease coverage by 0.37%.
The diff coverage is 86.71%.

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
- Coverage   96.82%   96.44%   -0.38%     
==========================================
  Files          45       47       +2     
  Lines        3087     3210     +123     
==========================================
+ Hits         2989     3096     +107     
- Misses         98      114      +16

Impacted Files	Coverage Δ
drf_spectacular/contrib/__init__.py	`100.00% <ø> (ø)`
drf_spectacular/contrib/rest_auth.py	`81.15% <81.15%> (ø)`
tests/conftest.py	`93.87% <88.88%> (-1.25%)`	⬇️
tests/__init__.py	`95.08% <90.62%> (-4.92%)`	⬇️
tests/contrib/test_rest_auth.py	`100.00% <100.00%> (ø)`
drf_spectacular/openapi.py	`93.11% <0.00%> (+0.17%)`	⬆️

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 0852def...5970984. Read the comment docs.

Closes tfranzel#113

jayvdb · 2020-07-19T14:20:30Z

Hmm. Forgot to add tests. Will amend tonight.

Use diff-match-patch to show differences only. Create baseline yml if it does not exist.

Output can vary between different versions of dependencies, and those differences may be of no consequence to the correctness of the test.

jayvdb · 2020-07-20T06:46:41Z

@tfranzel please reviewing the code contribution here in the PR, rather than your constant rewriting of other peoples code after merging. If there are stylistic issues, I am happy to adjust if that is communicated. Designs are better when people work together, rather than having ones persons design immediately 'cancelled' by someone else.

jayvdb · 2020-07-20T06:55:35Z

tests/__init__.py

+        dmp = diff_match_patch()
+        diff = dmp.diff_main(expected, generated)
+        dmp.diff_cleanupSemantic(diff)
+        patch = dmp.patch_toText(dmp.patch_make(diff))


This library is better than nothing, and quite good for short textual differences, especially within a single line, but the current results are far from perfect for large chunks of multiline differences.

The outstanding rest-polymorphic difference here is not exactly easy to grok.

Maybe the invocation can be improved by tweaking some of its options.

Doing a plain unified diff is also an option; that at least leverages existing brain circuitry for people familiar with diffs of YAML.

i just had a quick look if python's included batteries are good enough. looks good to me. i think we can refrain from adding a new dependency here.

diff = difflib.unified_diff( schema_ref.splitlines(True), schema_yml.decode().splitlines(True), ) diff = ''.join(diff) assert not diff, diff

tfranzel · 2020-07-20T14:15:10Z

sure thing. already working on the review. on your last PR i just changed a variable name and a style thing. i thought that it would be faster and that i wouldn't waste your time on a trivial second review round. sure i can include those OCD minors in the review process. whatever you prefer.

tfranzel

as requested quite a few comments/questions. good stuff though!

tfranzel · 2020-07-20T14:19:38Z

tests/__init__.py

+        dmp = diff_match_patch()
+        diff = dmp.diff_main(expected, generated)
+        dmp.diff_cleanupSemantic(diff)
+        patch = dmp.patch_toText(dmp.patch_make(diff))


i just had a quick look if python's included batteries are good enough. looks good to me. i think we can refrain from adding a new dependency here.

diff = difflib.unified_diff( schema_ref.splitlines(True), schema_yml.decode().splitlines(True), ) diff = ''.join(diff) assert not diff, diff

tfranzel · 2020-07-20T14:20:16Z

tests/__init__.py

+        raise RuntimeError(
+            f'{reference_filename} was not found. '
+            f'It has been created with the generated schema. '
+            f'Review carefully, as it is the baseline for subsequent checks.')


closing parenthesis on a new line please

tfranzel · 2020-07-20T14:25:40Z

tests/contrib/test_rest_auth.py

+
+
+@pytest.mark.contrib('rest_auth')
+def test_rest_auth_registration(no_warnings):


imho you can merge those two tests together with one yaml.

tfranzel · 2020-07-20T14:26:18Z

requirements/testing.txt

@@ -1,7 +1,8 @@
+diff-match-patch


we can remove it (see other comment)

tfranzel · 2020-07-20T14:26:53Z

requirements/optionals.txt

@@ -1,4 +1,6 @@
+django-allauth


please add working min versions

tfranzel · 2020-07-20T16:35:15Z

drf_spectacular/contrib/rest_auth.py

+from rest_framework import serializers
+
+try:
+    from allauth.account.app_settings import EMAIL_VERIFICATION, EmailVerificationMethod


please do local import and setup in the extensions instead (for contrib stuff). duplication seem minimal from a quick glance. python, basic django&drf and spectaculuar imports can stay of course.

i only did those fallbacks out of necessity in the tests when running with the packaging (missing-contrib) option. not really necessary here.

tfranzel · 2020-07-20T16:38:13Z

tests/conftest.py

    yield capsys
+
+    if request.node.rep_call.failed:


what is the rational behind this? why is it needed?

See commit messages please.

tfranzel · 2020-07-20T16:50:21Z

tests/contrib/test_rest_auth.py

+
+transforms = [
+    # User model first_name differences
+    lambda x: re.sub(r'(first_name:\n *type: string\n *maxLength:) 150', r'\g<1> 30', x, re.M),


afais those transforms do nothing. test pass without them.

They are required on different versions of django and drf. This is explained in the commit messages.

ah that makes sense. now i see. can you extend the comment with something like (changed in Django 3.1) to make it more obvious.

tests/__init__.py

tfranzel · 2020-07-20T16:59:00Z

tests/contrib/test_rest_auth.py

+    generator = SchemaGenerator(patterns=urlpatterns)
+    schema = generator.get_schema(request=None, public=True)
+
+    assert_schema(schema, 'tests/contrib/test_rest_auth.yml',


i prefer

assert_schema( schema, 'tests/contrib/test_rest_auth.yml', transforms=transforms )

or

assert_schema( schema, 'tests/contrib/test_rest_auth.yml', transforms=transforms )

depending on line length. i try not going over ~95 in general

jayvdb changed the title ~~contrib: Add rest-auth helpers~~ WIP: Add rest-auth helpers Jul 14, 2020

jayvdb commented Jul 14, 2020

View reviewed changes

drf_spectacular/contrib/rest_auth.py Outdated Show resolved Hide resolved

jayvdb force-pushed the contrib-rest-auth branch 3 times, most recently from 138e24e to d1c6141 Compare July 14, 2020 15:06

jayvdb changed the title ~~WIP: Add rest-auth helpers~~ Add rest-auth helpers Jul 14, 2020

jayvdb force-pushed the contrib-rest-auth branch 3 times, most recently from 4138555 to b8c425b Compare July 14, 2020 15:22

contrib: Add rest-auth support

1794cde

Closes tfranzel#113

jayvdb force-pushed the contrib-rest-auth branch from b8c425b to ec4c94c Compare July 20, 2020 03:22

jayvdb added 3 commits July 20, 2020 11:19

Add rest-auth tests

b70fef2

Improve schema difference test harness

a3c7b45

Use diff-match-patch to show differences only. Create baseline yml if it does not exist.

tests: Only force no warnings if test passed

5db8ddd

jayvdb force-pushed the contrib-rest-auth branch 3 times, most recently from 163288f to f948baa Compare July 20, 2020 06:03

jayvdb added 2 commits July 20, 2020 13:12

tests: Allow transformers on expected schemas

ccf6ca2

Output can vary between different versions of dependencies, and those differences may be of no consequence to the correctness of the test.

test_rest_auth: Add test schema transforms

5970984

jayvdb force-pushed the contrib-rest-auth branch from f948baa to 5970984 Compare July 20, 2020 06:18

jayvdb commented Jul 20, 2020

View reviewed changes

tfranzel requested changes Jul 20, 2020

View reviewed changes

tfranzel merged commit afb9424 into tfranzel:master Oct 13, 2020

jayvdb mentioned this pull request Oct 15, 2020

WIP: Add drf-gis fields #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rest-auth helpers #125

Add rest-auth helpers #125

jayvdb commented Jul 14, 2020

jayvdb commented Jul 14, 2020 •

edited

Loading

jayvdb Jul 14, 2020

jayvdb Jul 20, 2020

tfranzel Jul 20, 2020

jayvdb Jul 21, 2020

tfranzel Jul 21, 2020

jayvdb commented Jul 14, 2020

codecov bot commented Jul 14, 2020 •

edited

Loading

jayvdb commented Jul 19, 2020

jayvdb commented Jul 20, 2020

jayvdb Jul 20, 2020

tfranzel Jul 20, 2020

tfranzel commented Jul 20, 2020

tfranzel left a comment

tfranzel Jul 20, 2020

tfranzel Jul 20, 2020

tfranzel Jul 20, 2020

tfranzel Jul 20, 2020

tfranzel Jul 20, 2020

tfranzel Jul 20, 2020

tfranzel Jul 20, 2020

jayvdb Jul 21, 2020

tfranzel Jul 20, 2020

jayvdb Jul 21, 2020

tfranzel Jul 21, 2020

tfranzel Jul 20, 2020



		class DRFDefaultDetailSerializer(serializers.Serializer):
		detail = serializers.CharField(read_only=True, required=False)



		@pytest.mark.contrib('rest_auth')
		def test_rest_auth_registration(no_warnings):

Add rest-auth helpers #125

Add rest-auth helpers #125

Conversation

jayvdb commented Jul 14, 2020

jayvdb commented Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Jul 14, 2020

codecov bot commented Jul 14, 2020 • edited Loading

Codecov Report

jayvdb commented Jul 19, 2020

jayvdb commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfranzel commented Jul 20, 2020

tfranzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Jul 14, 2020 •

edited

Loading

codecov bot commented Jul 14, 2020 •

edited

Loading