Adding a fast sign-in with google #66

maayanhd · 2021-04-13T12:13:20Z

adding routing [urls.py][1] and auth-all of django in the [settings.py][2] file
made some minor changes in the [base.html][3]

closes #64

Make sure that these settings are defined in the admin:
client id: 209174926940-maavnu1qvcrcil3la6a5h96b593d0p84.apps.googleusercontent.com
secret key: ch-VdNjqiqtUT1zzrDlwHV6p
sites admin:

social application model:- make sure the site is found on the chosen sites

Leeadar · 2021-04-13T18:23:31Z

When I click on Login with Google I get this error:

nadavspinco

I got the same error as Leead.

maayanhd

@Leeadar and @nadavspinco It might have something to do with the registration of the sites and social app in the admin for configuration (linked to the way it configured through the settings file) I thought It will work without configuring it on every VM, if that is the case it might be better to give up on this PR, I will attach demonstration but it seems too much work for such a simple feature and because of google limitations it's not really practical at this point. @Yarboa @guy9050 @EdDev , right? I will demonstrate what is needed in my opinion to fix it, and maybe if there's a way to automate it will be smoother.

maayanhd · 2021-04-13T14:30:37Z

pickATrip_django_apps/admin.py

@@ -23,11 +24,20 @@ def __init__(self, general_model, admin_site):
        super(ListAdminMixin, self).__init__(general_model, admin_site)


+def is_related_to_social_signing(suspected_model):


This is a bad solution but I couldn't find any other solution to exclude these models relates to the 'sites' and 'all-auth' models of Django (since there is an auto-registration of those models and then I kept getting the Already registered exception)- please give me your input here - @EdDev @guy9050 @Yarboa

can you please elaborate more what the problem is and how does this function solve it?

The automation of the model registration suppose to register to admin only models that haven't been registered already, regarding the 4, 5 models provided by Django that are allowing social account logins (like google sign in) are already registered by Django and then the code tries to register it although it already has been registered, and this causes an exception. I couldn't find a solution so I enforced the automation part to avoid registering the 4, 5 problematic models that have been registered by Django to avoid the problem using an ugly if statement that assuring the model that is registered is not one of these problematic ones (it might have something to do with the fact the social account models are registered differently because it happened automatically after adding the new settings to support social login)

Yarboa · 2021-04-18T07:01:46Z

pickATrip_django_apps/admin.py

-        admin.site.register(model, admin_class)
-    except admin.sites.AlreadyRegistered:
-        pass
+    if not is_related_to_social_signing(model):


This is not clear to me, Does the app could run both?
with the social auth and without?

no, It's somehow registered automatically, so In order to avoid conflicts of registering twice I used this bad solution, but If it makes too much trouble we can give up on this PR, the other mentors have told me it's not a great idea
(not in these words).
And I followed a tutorial so I can't assume it would work without it.

Sorry, i was asking misleading question,
In case no social media is set in admin, will it work the same w/o this PR?

@Yarboa sorry I'm still not entirely understand your question, what do you mean by 'social media is set in admin'? I wasn't setting anything since Django is registering the social media models related to OAuth2 of google, and these specific models are making trouble with the auto-model registration code since it's trying to register the models again. I don't think I can give up on those settings (and this is what generates these social media models of Django), I followed a tutorial so I believe every setting is crucial, and Django is having a very specific way to do so, as I read from several sources.

There is pip install of django-allauth
And in App/url.py

This is default user, no option to work with non allauth.urls?
path('accounts/', include('allauth.urls'))

Ok looking here https://django-
allauth.readthedocs.io/en/latest/overview.html

Signup of both local and social accounts, did you check that you can login local?

How do you plan to test this feature?

NOTE: I suggest to describe better in the comment what are the capabilities,

pickATrip_django_apps/templates/pickATrip_django_apps/base.html

maayanhd · 2021-04-29T08:48:20Z

@nadavspinco

This check fails for some reason, isn't it suppose to fail if the view function fails, or what exactly, because the site is working properly but still this fails.

Yarboa · 2021-05-03T16:22:38Z

@nadavspinco

This check fails for some reason, isn't it suppose to fail if the view function fails, or what exactly, because the site is working properly but still this fails.

Did you try pressing homepage, about? while server is up?

nadavspinco · 2021-05-03T16:38:26Z

picATrip/urls.py

+        'about/',
+        TemplateView.as_view(template_name="pickATrip_django_apps/about.html"),
+        name='about',
+    ),


Those changes fail on tests. if we use Template.as_view I need to change the test because Template.as_view returns a different function on memory and python refers to this as different functions.
@maayanhd There is a reason you changed that?

Actually, I did this at first because I thought it would be a better way, but I couldn't find if this is the case and haven't think of the fact it might fail future tests, thanks for clarifying.

Yarboa · 2021-05-06T13:06:35Z

users/tests/test_allauth.py

+        first_name = "明"
+        last_name = "小"


I would advise using english here
and auto generate name last name

Yarboa · 2021-05-06T13:17:22Z

users/tests/test_allauth.py

+    ACCOUNT_SIGNUP_FORM_CLASS=None,
+    ACCOUNT_EMAIL_VERIFICATION=account_settings.EmailVerificationMethod.MANDATORY,
+)
+class GoogleTests(OAuth2TestsMixin, TestCase):


we are using django-pytest we are not using TestCases
My intention was to receive ideas and work in django-pytest not to copy
sorry mate

To be honest I don't find much purpose in not reusing parts of complex code for testing integration of Django with Google API without needing to write from a scratch method by method, this test uses preprepared objects to ease the test of the sign in with google and it has been written for the purpose of testing this feature, I do see value in making modifications from using test case to PyTest but further than that not sure why is it bad reusing code if it feets my purposes, If something I thought not doing everything is part of a developer's job, to save valuable developing time, so there's time for the more important stuff. That's my opinion but that's only one opinion after all.

To be honest I don't find much purpose in not reusing parts of complex code for testing integration of Django with Google API without needing to write from a scratch method by method, this test uses preprepared objects to ease the test of the sign in with google and it has been written for the purpose of testing this feature, I do see value in making modifications from using test case to PyTest but further than that not sure why is it bad reusing code if it feets my purposes, If something I thought not doing everything is part of a developer's job, to save valuable developing time,

I am not sure it is not helping your knowledge and skills as SW dev.

so there's time for the more important stuff. That's my opinion but that's only one opinion after all.

You are correct, but i would advise to maintain testing standards.
Please rethink even in cost of less test coverage, to work with django-pytest standards

Yarboa · 2021-05-09T13:32:47Z

users/tests/test_allauth.py

+<<<<<<< HEAD
+class GoogleTests(OAuth2TestsMixin):


You did not rebase correctly

Yes not sure how this file got uploaded I erased it some kind of mistake of course..

Yarboa · 2021-05-09T13:32:59Z

users/tests/test_allauth.py

+
+    @pytest.fixture
+    def unverified_mail_mocked_response(
+<<<<<<< HEAD


same as above

Yarboa · 2021-05-09T13:33:15Z

users/tests/test_allauth.py

+=======
+<<<<<<< HEAD


same as above

Yarboa · 2021-05-09T13:33:25Z

users/tests/test_allauth.py

+            name,
+            email,
+            unverified_email_val,
+>>>>>>> d3ee076 (fix)


same as above

Yarboa · 2021-05-09T13:33:41Z

users/tests/test_allauth.py

+        name="Raymond Penners",
+        email="[email protected]",
+        verified_email=True,
+>>>>>>> 42f9e5e (Fixtures and tests for sign in with google feature)


same as above

Yarboa · 2021-05-09T13:34:55Z

users/tests/test_allauth.py

+                name,
+                email,
+                given_name,
+<<<<<<< HEAD


Please review the file
You can not force push without complete rebase

Please review all <<< ====
And do manual merge and then run tests, and commit push

- Adding routing [urls.py][1] and auth-all of django in the [settings.py][2] file - Made some minor changes in the [base.html][3] Fast sign in with google for test users (in production adding gmail email for testing the feature) closes # 64

- Adding [allauth_fixtures][0] file for automating the site registation and social account in order to avoid doing so locally on each VM - Adding [test_allauth][1] file for testing the sign in eith google feature based on the tests file appeared in google provider tests in the allauth django project

- Added DB annottion of pytest due to a conflit occurs after adding the sign in with google feature

nadavspinco

I got this execption.

maayanhd · 2021-05-14T07:24:33Z

@nadavspinco Because you didn't follow the instructions of adding a social app, anyway, it looks like this PR won't be merged as it is, unfortunately.

maayanhd requested review from Shoval-Ovadia and Leeadar and removed request for Shoval-Ovadia April 13, 2021 12:14

maayanhd force-pushed the google-fast-signing branch from 2258bc5 to 6c9f143 Compare April 13, 2021 12:32

maayanhd changed the title ~~Adding a fast sign-in with google~~ Adding a fast sign-in with google and small content changes Apr 13, 2021

maayanhd force-pushed the google-fast-signing branch from 6c9f143 to 1f63131 Compare April 13, 2021 13:10

maayanhd self-assigned this Apr 13, 2021

maayanhd changed the title ~~Adding a fast sign-in with google and small content changes~~ Adding a fast sign-in with google Apr 13, 2021

maayanhd force-pushed the google-fast-signing branch from 1f63131 to 3a39399 Compare April 13, 2021 13:26

maayanhd requested review from Yarboa, guy9050 and nadavspinco April 13, 2021 13:27

maayanhd force-pushed the google-fast-signing branch from 307f4dd to c156572 Compare April 13, 2021 14:33

nadavspinco reviewed Apr 14, 2021

View reviewed changes

maayanhd commented Apr 14, 2021

View reviewed changes

maayanhd added the enhancement New feature or request label Apr 16, 2021

Yarboa reviewed Apr 18, 2021

View reviewed changes

Yarboa reviewed Apr 19, 2021

View reviewed changes

pickATrip_django_apps/templates/pickATrip_django_apps/base.html Show resolved Hide resolved

maayanhd added the Status/Backlog label Apr 21, 2021

maayanhd added Status/In-Progress and removed Status/Backlog labels Apr 29, 2021

maayanhd force-pushed the google-fast-signing branch 2 times, most recently from bb5ccd6 to c636702 Compare April 29, 2021 08:46

nadavspinco reviewed May 3, 2021

View reviewed changes

maayanhd force-pushed the google-fast-signing branch 2 times, most recently from 9aca1f8 to ddb8057 Compare May 6, 2021 10:57

maayanhd force-pushed the google-fast-signing branch from ddb8057 to 42f9e5e Compare May 6, 2021 11:08

maayanhd removed Status/In-Progress enhancement New feature or request labels May 6, 2021

Yarboa reviewed May 6, 2021

View reviewed changes

maayanhd force-pushed the google-fast-signing branch 3 times, most recently from 93ff587 to 2ce1212 Compare May 9, 2021 09:39

Yarboa reviewed May 9, 2021

View reviewed changes

maayanhd force-pushed the google-fast-signing branch 3 times, most recently from 53d4e73 to a87ee9d Compare May 11, 2021 09:23

maayanhd added 3 commits May 11, 2021 12:35

Adding a fast sign-in with google

ccf69ff

- Adding routing [urls.py][1] and auth-all of django in the [settings.py][2] file - Made some minor changes in the [base.html][3] Fast sign in with google for test users (in production adding gmail email for testing the feature) closes # 64

Fixing user_views_tests

ccdf698

- Added DB annottion of pytest due to a conflit occurs after adding the sign in with google feature

maayanhd force-pushed the google-fast-signing branch from a87ee9d to ccdf698 Compare May 11, 2021 09:35

nadavspinco reviewed May 13, 2021

View reviewed changes

maayanhd added the wontfix This will not be worked on label May 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a fast sign-in with google #66

Adding a fast sign-in with google #66

maayanhd commented Apr 13, 2021 •

edited

Loading

Leeadar commented Apr 13, 2021

nadavspinco left a comment •

edited

Loading

maayanhd left a comment •

edited

Loading

maayanhd Apr 13, 2021

guy9050 Apr 18, 2021

maayanhd Apr 29, 2021 •

edited

Loading

Yarboa Apr 18, 2021

maayanhd Apr 18, 2021 •

edited

Loading

Yarboa Apr 18, 2021

maayanhd Apr 18, 2021 •

edited

Loading

Yarboa Apr 19, 2021 •

edited

Loading

maayanhd commented Apr 29, 2021

Yarboa commented May 3, 2021 •

edited

Loading

nadavspinco May 3, 2021 •

edited

Loading

maayanhd May 4, 2021

Yarboa May 6, 2021

Yarboa May 6, 2021

maayanhd May 6, 2021 •

edited

Loading

Yarboa May 6, 2021

Yarboa May 9, 2021

maayanhd May 10, 2021

Yarboa May 9, 2021

Yarboa May 9, 2021

Yarboa May 9, 2021

Yarboa May 9, 2021

Yarboa May 9, 2021

nadavspinco left a comment

maayanhd commented May 14, 2021 •

edited

Loading

		@@ -23,11 +24,20 @@ def __init__(self, general_model, admin_site):
		super(ListAdminMixin, self).__init__(general_model, admin_site)


		def is_related_to_social_signing(suspected_model):

		<<<<<<< HEAD
		class GoogleTests(OAuth2TestsMixin):

		=======
		<<<<<<< HEAD

Adding a fast sign-in with google #66

Are you sure you want to change the base?

Adding a fast sign-in with google #66

Conversation

maayanhd commented Apr 13, 2021 • edited Loading

Leeadar commented Apr 13, 2021

nadavspinco left a comment • edited Loading

Choose a reason for hiding this comment

maayanhd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maayanhd Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maayanhd Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maayanhd Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

Yarboa Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

maayanhd commented Apr 29, 2021

Yarboa commented May 3, 2021 • edited Loading

nadavspinco May 3, 2021 • 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

maayanhd May 6, 2021 • 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

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

nadavspinco left a comment

Choose a reason for hiding this comment

maayanhd commented May 14, 2021 • edited Loading

maayanhd commented Apr 13, 2021 •

edited

Loading

nadavspinco left a comment •

edited

Loading

maayanhd left a comment •

edited

Loading

maayanhd Apr 29, 2021 •

edited

Loading

maayanhd Apr 18, 2021 •

edited

Loading

maayanhd Apr 18, 2021 •

edited

Loading

Yarboa Apr 19, 2021 •

edited

Loading

Yarboa commented May 3, 2021 •

edited

Loading

nadavspinco May 3, 2021 •

edited

Loading

maayanhd May 6, 2021 •

edited

Loading

maayanhd commented May 14, 2021 •

edited

Loading