Login with GitHub

[SalmanAsh]

This change is 

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 10 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 14 at r1 (raw file):

from rest_framework import status

import settings

don't import settings like this. instead

from django.conf import settings

Code quote:

import settings

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 14 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 22 at r1 (raw file):

# pylint: disable-next=missing-class-docstring,too-many-ancestors
class ContributorViewSet(ModelViewSet[User, Contributor]):
    http_method_names = ["get", "post", "delete"]

comment out post for now

http_method_names = ["get"]  # "post"

---

api/views/contributor.py line 27 at r1 (raw file):

    queryset = Contributor.objects.all()

    @action(detail=False, methods=["get"])

# TODO: delete custom action and override default create action. 
@action(detail=False, methods=["get"])

Code quote:

    @action(detail=False, methods=["get"])

---

api/views/contributor.py line 29 at r1 (raw file):

    @action(detail=False, methods=["get"])
    def log_into_github(self, request: Request):
        """Users can login using their existing github account"""

Replace "LINK HERE" with the link to GH docs that explains how users can authorise 3rd party apps to access their GH account.

"""
Creates a new contributor or updates an existing contributor. This requires users to authorise us to read their GitHub account.

LINK HERE
"""

Code quote:

"""Users can login using their existing github account"""

---

api/views/contributor.py line 31 at r1 (raw file):

        """Users can login using their existing github account"""
        # Get code from login request
        if not request.GET.get("code"):

store in a variable

code = request.GET.get("code")

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 15 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 35 at r1 (raw file):

        # Get user access Token
        access_token_request = requests.post(

response = requests.post(

Code quote:

        access_token_request = requests.post(

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 16 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 51 at r1 (raw file):

        # Code expired
        if "access_token" not in auth_data:

Does this happen? It makes more sense that GH would just return an error status code rather than returning a success status code with missing data.

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 17 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 64 at r1 (raw file):

            headers={
                "Accept": "application/json",
                "Authorization": f"{token_type} {access_token}",

simplify to:

f"{auth_data['token_type']} {auth_data['access_token']}"

Code quote:

f"{token_type} {access_token}"

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 18 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 60 at r1 (raw file):

        # Get user's information
        user_data_request = requests.get(

response = requests.get(

Code quote:

        user_data_request = requests.get(

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 19 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor_test.py line 14 at r1 (raw file):

from rest_framework import status

import settings

see comment about how to import settings

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 20 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 102 at r1 (raw file):

        return Response(
            status=status.HTTP_451_UNAVAILABLE_FOR_LEGAL_REASONS,
        )

all of this should be replaced with

serializer = self.get_serializer(data=request.data)
        serializer.is_valid(raise_exception=True)
        self.perform_create(serializer)
        return Response(serializer.data, status=status.HTTP_201_CREATED, headers=self.get_success_headers(serializer.data))

Code quote:

        user_data = user_data_request.json()
        if not user_data["email"]:
            return Response(
                data="Email null",
                status=status.HTTP_451_UNAVAILABLE_FOR_LEGAL_REASONS,
            )

        # Check if user is already a contributor
        gh_id = user_data["id"]
        contributor_data = {
            "id": gh_id,
            "email": user_data["email"],
            "name": user_data["name"],
            "location": user_data["location"],
            "html_url": user_data["html_url"],
            "avatar_url": user_data["avatar_url"],
        }

        try:
            # Update an existing contributor
            contributor = Contributor.objects.get(pk=gh_id)
            serializer = ContributorSerializer(
                contributor, data=contributor_data
            )
        except Contributor.DoesNotExist:
            # Create a new contributor
            serializer = ContributorSerializer(data=contributor_data)

        if serializer.is_valid():
            serializer.save()
            return Response(status=status.HTTP_200_OK)
        return Response(
            status=status.HTTP_451_UNAVAILABLE_FOR_LEGAL_REASONS,
        )

[SKairinos]

Reviewable status: 7 of 9 files reviewed, 20 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 102 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

all of this should be replaced with

serializer = self.get_serializer(data=request.data)
        serializer.is_valid(raise_exception=True)
        self.perform_create(serializer)
        return Response(serializer.data, status=status.HTTP_201_CREATED, headers=self.get_success_headers(serializer.data))

you should then override the create method in the contributor serializer to check if the contributor exists or not.

[SKairinos]

Reviewed 3 of 8 files at r2, all commit messages.
Reviewable status: 5 of 10 files reviewed, 15 unresolved discussions (waiting on @SalmanAsh)

---

settings.py line 30 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

GH_CLIENT_ID = os.getenv("GH_CLIENT_ID", "replace-me")

you still need to rename the setting

---

settings.py line 31 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

GH_CLIENT_SECRET = os.getenv("GH_CLIENT_SECRET", "replace-me")

you still need to rename the setting

[SKairinos]

Reviewed 1 of 8 files at r2.
Reviewable status: 6 of 10 files reviewed, 17 unresolved discussions (waiting on @SalmanAsh)

---

api/serializers/contributor.py line 5 at r2 (raw file):

Created on 12/07/2024 at 14:07:59(+01:00).
"""
from typing import Any, Dict

import typing as t

---

api/serializers/contributor.py line 18 at r2 (raw file):

        model = Contributor
        fields = ["id", "email", "name", "location", "html_url", "avatar_url"]
        extra_kwargs: Dict[str, Dict[str, Any]] = {"id": {"validators": []}}

xtra_kwargs: t.Dict[str, t.Dict[str, t.Any]]

Code quote:

xtra_kwargs: Dict[str, Dict[str, Any]]

[SKairinos]

Reviewable status: 6 of 10 files reviewed, 18 unresolved discussions (waiting on @SalmanAsh)

---

api/serializers/contributor.py line 23 at r2 (raw file):

        try:
            # Update an existing contributor
            contributor = Contributor.objects.get(pk=validated_data["id"])

id=validated_data["id"]

[SKairinos]

Reviewable status: 6 of 10 files reviewed, 19 unresolved discussions (waiting on @SalmanAsh)

---

api/serializers/contributor.py line 48 at r2 (raw file):

                html_url=validated_data["html_url"],
                avatar_url=validated_data["avatar_url"],
            )

contributor = Contributor.objects.create(**validated_data)

Code quote:

            contributor = Contributor.objects.create(
                id=validated_data["id"],
                email=validated_data["email"],
                name=validated_data["name"],
                location=validated_data["location"],
                html_url=validated_data["html_url"],
                avatar_url=validated_data["avatar_url"],
            )

[SKairinos]

Reviewed 1 of 8 files at r2.
Reviewable status: 7 of 10 files reviewed, 9 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 67 at r2 (raw file):

        response = requests.get(
            url="https://api.github.com/user",
            headers={

specify api version. always do this

[SKairinos]

Reviewed 2 of 8 files at r2.
Reviewable status: 9 of 10 files reviewed, 7 unresolved discussions (waiting on @SalmanAsh)

[SKairinos]

Reviewable status: 9 of 10 files reviewed, 8 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor_test.py line 43 at r2 (raw file):

            self.reverse_action(
                "log_into_github",
            ),

one line. do this everywhere.

self.reverse_action("log_into_github"),

Code quote:

            self.reverse_action(
                "log_into_github",
            ),

[SKairinos]

Reviewable status: 9 of 10 files reviewed, 13 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor_test.py line 44 at r2 (raw file):

                "log_into_github",
            ),
            {"code": ""},

data={"code": ...},

Code quote:

{"code": ""}

---

api/views/contributor_test.py line 60 at r2 (raw file):

                    "log_into_github",
                ),
                {"code": "3e074f3e12656707cf7f"},

data={"code": ...},

---

api/views/contributor_test.py line 92 at r2 (raw file):

                    "log_into_github",
                ),
                {"code": "3e074f3e12656707cf7f"},

data={"code": ...},

---

api/views/contributor_test.py line 143 at r2 (raw file):

                        "log_into_github",
                    ),
                    {"code": "3e074f3e12656707cf7f"},

data={"code": ...},

---

api/views/contributor_test.py line 204 at r2 (raw file):

                        "log_into_github",
                    ),
                    {"code": "3e074f3e12656707cf7f"},

data={"code": ...},

[SKairinos]

Reviewable status: 9 of 10 files reviewed, 17 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor_test.py line 64 at r2 (raw file):

            )

            requests_post.assert_called_once_with(

make a helper function which makes this assertion so you can call the helper function multiple time instead

---

api/views/contributor_test.py line 97 at r2 (raw file):

            )

            requests_post.assert_called_once_with(

make a helper function which makes this assertion so you can call the helper function multiple time instead

---

api/views/contributor_test.py line 147 at r2 (raw file):

                )

                requests_post.assert_called_once_with(

make a helper function which makes this assertion so you can call the helper function multiple time instead

---

api/views/contributor_test.py line 207 at r2 (raw file):

                )

                requests_post.assert_called_once_with(

make a helper function which makes this assertion so you can call the helper function multiple time instead

[SKairinos]

Reviewed 3 of 4 files at r3, all commit messages.
Reviewable status: 9 of 10 files reviewed, 11 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor.py line 47 at r3 (raw file):

            headers={
                "Accept": "application/json",
                "X-GitHub-Api-Version": "2022-11-28",

this is not github's api - no need to specify version.

"https://github.com" vs "https://api.github.com"

[SKairinos]

Reviewable status: 9 of 10 files reviewed, 7 unresolved discussions (waiting on @SalmanAsh)

---

api/views/contributor_test.py line 46 at r3 (raw file):

        )

    def verify_api_call(self, request, url):

1. helpers should be private
2. always add type annotations if the type is not specified but known
3. make code an arg
4. rename to _assert_request_github_access_token

def _assert_request_github_access_token(self, request: Mock, url: str, code: str): ...

---

api/views/contributor_test.py line 49 at r3 (raw file):

        """API call is correctly executed."""
        request.assert_called_once_with(
            url=url,

url = "https://github.com/login/oauth/access_token"

---

api/views/contributor_test.py line 52 at r3 (raw file):

            headers={
                "Accept": "application/json",
                "X-GitHub-Api-Version": "2022-11-28",

delete this

---

api/views/contributor_test.py line 57 at r3 (raw file):

                "client_id": settings.GH_CLIENT_ID,
                "client_secret": settings.GH_CLIENT_SECRET,
                "code": "3e074f3e12656707cf7f",

use code arg

---

api/views/contributor_test.py line 149 at r3 (raw file):

                )

                requests_get.assert_called_once_with(

create another helper called def _assert_request_github_user(self, auth: str) and call it

---

api/views/contributor_test.py line 204 at r3 (raw file):

                )

                requests_get.assert_called_once_with(

create another helper called def _assert_request_github_user(self, auth: str) and call it

[SKairinos]

Reviewed 1 of 4 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @SalmanAsh)

[SKairinos]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SalmanAsh)

[SKairinos]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SalmanAsh)