New flow changes #100
New flow changes #100
Conversation
| @@ -5,15 +5,15 @@ meta { | |||
| } | |||
|
|
|||
| post { | |||
| url: {{baseUrl}}/accounts/register | |||
| url: http://localhost:8000/accounts/register | |||
There was a problem hiding this comment.
It doeesn't seem to pull it automatically though, It's better just to use the One that is going to be most often used with in development?
| } | ||
|
|
||
| post { | ||
| url: http://127.0.0.1:8000/accounts/check-SHA512-for-account/ |
| } | ||
|
|
||
| post { | ||
| url: http://127.0.0.1:8000/accounts/register-SHA512-for-account/ |
| } | ||
|
|
||
| post { | ||
| url: http://127.0.0.1:8000/persistence/characters/GenForkToken |
| @@ -5,11 +5,11 @@ meta { | |||
| } | |||
|
|
|||
| get { | |||
| url: {{baseUrl}}/persistence/characters | |||
| url: http://127.0.0.1:8000/persistence/characters | |||
| } | ||
|
|
||
| get { | ||
| url: http://localhost:8000/persistence/characters/compatibleToken?character_sheet_version=1.0.0 |
| operations = [ | ||
| migrations.CreateModel( | ||
| name='SHA512Token', | ||
| fields=[ |
There was a problem hiding this comment.
are these all the fields that we migrate? or are there more?
There was a problem hiding this comment.
idk,This is automatically generated code
|
|
||
| character_sheet_version = query_serializer.validated_data["character_sheet_version"] | ||
|
|
||
| return Character.objects.filter( |
There was a problem hiding this comment.
shouldn't this be handled with a trycatch before hand?
| @@ -0,0 +1,3 @@ | |||
| meta { | |||
| name: SHAChecks | |||
There was a problem hiding this comment.
don't commit bruno files. It was good before we had any auto documentation on the endpoints but now it will just confuse other developers. I'd say you can even remove the whole api-collection folder
| ConfirmAccountView, | ||
| LoginWithCredentialsView, | ||
| LoginWithTokenView, | ||
| RegisterAccountView, | ||
| RegisterSHA512ForAccount, |
There was a problem hiding this comment.
I think Snep said SHA512 wasn't good enough for us. I would wait for his input here
|
|
||
|
|
||
| class RegisterSHA512ForAccount(APIView): | ||
| class InputSerializer(serializers.Serializer): |
There was a problem hiding this comment.
don't create serializers inside the view class. There is a serializers file for this purpose. Everything on their own file
There was a problem hiding this comment.
also the name for this serializer sucks. Follow the convention of the other serializers in the project
|
|
||
| def post(self, request, *args, **kwargs): | ||
| user: Account = request.user | ||
| if not request.auth: |
There was a problem hiding this comment.
since this view doesn't override the permission_classes field, it will use the default one which is IsAuthenticatedand handles the case where not valid auth token is passed, so this would not be needed.
| try: | ||
| account = Account.objects.get(unique_identifier=unique_id) | ||
| except Account.DoesNotExist: | ||
| return Response({"exists": False}, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
even tho a 404 here would explain everything already, I think it is more sensible to respond with 401 unaunthenticated and the message Token has expired or is invalid
| try: | ||
| account = Account.objects.get(unique_identifier=account_uuid) | ||
| except Account.DoesNotExist: | ||
| return Response({"error": "Account not found"}, status=status.HTTP_404_NOT_FOUND) |
There was a problem hiding this comment.
same here, just return 401 and say the token was invalid
| serializer_class = CharacterSerializer | ||
|
|
||
| def post(self, request): | ||
| server_id = request.data.get("fork_compatibility") |
There was a problem hiding this comment.
this is not server id, use fork_compatibility. Even I got confused when reading the code because servers will have a server id eventually, which is to individually identify each of them and this is not it
| data_with_extras["fork_compatibility"] = server_id # Enforce fork from token | ||
|
|
||
| serializer = self.serializer_class(data=data_with_extras) | ||
| serializer.account = account # type: ignore |
There was a problem hiding this comment.
what happened with the typing here?
|
|
||
| # Add fork_compatibility from the token and character_sheet_version from query | ||
| query_data = { | ||
| "character_sheet_version": self.request.query_params.get("character_sheet_version"), |
There was a problem hiding this comment.
just to confirm this was your intention, this means you have to add ?character_sheet_version=v1.0.0 to the URL
| except Account.DoesNotExist: | ||
| return Response({"error": "Account not found"}, status=status.HTTP_404_NOT_FOUND) | ||
|
|
||
| # Try to get character, otherwise create a new one |
There was a problem hiding this comment.
maybe this should just patch existing ones. Why create when you have an endpoint for that?
Is missing the job to clear out the old sha tokens
And maybe the time Expiry for character settings token?