refactor(repository): Tokens repository JSON backend #18
No reviewers
Labels
No Label
Bug
Contributions welcome
Did not do
Duplicate
Feature
Module
Backups
Module
GraphQL
Priority
High
Priority
Low
Priority
Medium
Refactor
Severity
High
Severity
Low
Severity
Medium
No Milestone
No project
No Assignees
3 Participants
Notifications
Total Time Spent: 18 hours 47 minutes
Due Date
def
18 hours 47 minutes
Dependencies
No dependencies set.
Reference: SelfPrivacy/selfprivacy-rest-api#18
Loading…
Reference in New Issue
There is no content yet.
Delete Branch "def/jobs-repo"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
@ -0,0 +103,4 @@
with ReadUserData(UserDataFiles.TOKENS) as tokens_file:
if tokens_file["recovery_token"] is None:
return
And what do we return?
@ -0,0 +109,4 @@
key=tokens_file["recovery_token"]["token"],
created_at=tokens_file["recovery_token"]["date"],
expires_at=tokens_file["recovery_token"]["expitation"],
uses_left=tokens_file["recovery_token"]["uses_left"],
Will raise KeyError.
@ -0,0 +121,4 @@
) -> RecoveryKey:
"""Create the recovery key"""
recovery_key = RecoveryKey.generate(expiration=None, uses_left=None)
Why passing None?
@ -0,0 +153,4 @@
def delete_new_device_key(self) -> None:
"""Delete the new device key"""
with WriteUserData(UserDataFiles.TOKENS) as tokens_file:
tokens_file.pop("new_device")
Will raise KeyError. Use this instead:
WIP: refactor: divided API into business logic and repositoryto WIP: refactor(repository): Tokens repository JSON backend@ -0,0 +52,4 @@
for userdata_token in tokens_file["tokens"]:
tokens_list.append(
Token(
token=userdata_token.token,
Are you sure this notation will work? Not consistent with the previous functions.
@ -0,0 +81,4 @@
if userdata_token["token"] == input_token:
tokens_file["tokens"].remove(
userdata_token
) # Allah, i pray it works
At SelfPrivacy, we do not engage in politics and religion. Pray to your unit tests that will check this code.
@ -0,0 +135,4 @@
def use_mnemonic_recovery_key(self, mnemonic_phrase: str, name: str) -> Token:
"""Use the mnemonic recovery key and create a new token with the given name"""
recovery_key = self.get_recovery_key() # self ?
yes, self, as the methods are not static
@ -0,0 +143,4 @@
if not recovery_key.is_valid():
return None
if recovery_key is None:
why two same checks?
@ -0,0 +155,4 @@
if phrase_bytes != recovery_token:
return None
new_recovery_key = RecoveryKey.generate()
Why generating a new recovery key? You need a token here.
@ -0,0 +161,4 @@
tokens["tokens"].append(
{
"token": new_recovery_key.key,
# "name": new_recovery_key.name, what???? there is no name
There is a name.
@ -0,0 +181,4 @@
tokens_file["new_device"] = {
"token": new_device_key.key,
"data": new_device_key.created_at,
"expiration": new_device_key.expires_at,
Are you sure the dates will serialize correctly? They are not strings, write tests to check the datetime formats.
@ -0,0 +194,4 @@
def use_mnemonic_new_device_key(self, mnemonic_phrase: str, name: str) -> None:
"""Use the mnemonic new device key"""
...
missing implementation?
Also, do not leave descriptions of your issues and PRs empty. Take a look at the latest NaiJi's PR: kherel/selfprivacy.org.app#145
I think it's also important to keep in mind that if there is anything to put in a description for a PR at all, it has to be also contained in the commits for the merge themselves, for example, if you PR one huge commit and put a list of changes in your PR's description, the commit has to contain the metadata as well.
To put it simply:
NOT:
DO THIS INSTEAD:
For better understanding read commits' descriptions of the PR @inex has mentioned and compare to the PR descriptons.
At least it's how I am now trying to do that. It's up on us to discuss if it's productive or not
d8b180c626
to80a3750d92
Taking this PR over.
Ok, here's the final review.
datetime
s. Serialization format is incosistent. Test did not cover different datetime formats on input.KeyError
s. Shipping this code is dangerous. Your tests did not cover this. Tests MUST cover basic edge cases on input data. Like:Error
in their name.Error
s andException
s are semantically different objects.WIP: refactor(repository): Tokens repository JSON backendto refactor(repository): Tokens repository JSON backend