refactor(repository): Tokens repository JSON backend #18

Merged
inex merged 12 commits from def/jobs-repo into master 2022-11-16 19:12:39 +02:00
Collaborator
There is no content yet.
inex was assigned by def 2022-10-27 23:54:04 +03:00
def added 2 commits 2022-10-27 23:54:05 +03:00
def requested review from inex 2022-10-27 23:54:21 +03:00
inex requested changes 2022-10-28 10:45:22 +03:00
@ -0,0 +103,4 @@
with ReadUserData(UserDataFiles.TOKENS) as tokens_file:
if tokens_file["recovery_token"] is None:
return

And what do we return?

And what do we return?
def marked this conversation as resolved
@ -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.

Will raise KeyError.
def marked this conversation as resolved
@ -0,0 +121,4 @@
) -> RecoveryKey:
"""Create the recovery key"""
recovery_key = RecoveryKey.generate(expiration=None, uses_left=None)

Why passing None?

Why passing None?
def marked this conversation as resolved
@ -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:

if "new_device" in tokens:
            del tokens["new_device"]
Will raise KeyError. Use this instead: ```py if "new_device" in tokens: del tokens["new_device"] ```
def marked this conversation as resolved
inex changed title from WIP: refactor: divided API into business logic and repository to WIP: refactor(repository): Tokens repository JSON backend 2022-10-28 11:31:16 +03:00
inex removed their assignment 2022-10-28 13:25:35 +03:00
def added 1 commit 2022-10-29 03:50:27 +03:00
def requested review from inex 2022-10-30 18:38:50 +02:00
inex requested changes 2022-10-31 10:01:46 +02:00
@ -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.

Are you sure this notation will work? Not consistent with the previous functions.
def marked this conversation as resolved
@ -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.

At SelfPrivacy, we do not engage in politics and religion. Pray to your unit tests that will check this code.
def marked this conversation as resolved
@ -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

yes, self, as the methods are not static
def marked this conversation as resolved
@ -0,0 +143,4 @@
if not recovery_key.is_valid():
return None
if recovery_key is None:

why two same checks?

why two same checks?
def marked this conversation as resolved
@ -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.

Why generating a new recovery key? You need a token here.
def marked this conversation as resolved
@ -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.

There is a name.
def marked this conversation as resolved
@ -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.

Are you sure the dates will serialize correctly? They are not strings, write tests to check the datetime formats.
def marked this conversation as resolved
@ -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?

missing implementation?
def marked this conversation as resolved

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

Also, do not leave descriptions of your issues and PRs empty. Take a look at the latest NaiJi's PR: https://git.selfprivacy.org/kherel/selfprivacy.org.app/pulls/145
Collaborator

Also, do not leave descriptions of your issues and PRs empty.

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:

commit:
feat: Implement cool stuff

PR description:
By stuff I mean this and that! Also 
- endpoints are changed
- validations are changed
- i adopted a cat
- i found out that tea with 3 spoons of sugar is the best

DO THIS INSTEAD:

commit:
feat: Implement cool stuff

By stuff I mean this and that! Also 
- endpoints are changed
- validations are changed
- i adopted a cat
- i found out that tea with 3 spoons of sugar is the best

PR description (just copypaste from the commit itself):
By stuff I mean this and that! Also 
- endpoints are changed
- validations are changed
- i adopted a cat
- i found out that tea with 3 spoons of sugar is the best

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

> Also, do not leave descriptions of your issues and PRs empty. 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:** ``` commit: feat: Implement cool stuff PR description: By stuff I mean this and that! Also - endpoints are changed - validations are changed - i adopted a cat - i found out that tea with 3 spoons of sugar is the best ``` **DO THIS INSTEAD:** ``` commit: feat: Implement cool stuff By stuff I mean this and that! Also - endpoints are changed - validations are changed - i adopted a cat - i found out that tea with 3 spoons of sugar is the best PR description (just copypaste from the commit itself): By stuff I mean this and that! Also - endpoints are changed - validations are changed - i adopted a cat - i found out that tea with 3 spoons of sugar is the best ``` 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
def added the due date 2022-11-08 2022-11-04 01:24:17 +02:00
def started working 2022-11-04 01:24:46 +02:00
def force-pushed def/jobs-repo from d8b180c626 to 80a3750d92 2022-11-04 01:29:41 +02:00 Compare
def cancelled time tracking 2022-11-04 01:34:58 +02:00
def started working 2022-11-04 11:11:11 +02:00
def stopped working 2022-11-04 11:16:14 +02:00
5min 3s
def added spent time 2022-11-04 11:16:27 +02:00
10min
def started working 2022-11-04 11:24:56 +02:00
def stopped working 2022-11-04 12:18:51 +02:00
53min 55s
def started working 2022-11-04 13:07:14 +02:00
def stopped working 2022-11-04 13:25:20 +02:00
18min 6s
def started working 2022-11-04 14:23:36 +02:00
def stopped working 2022-11-04 14:44:34 +02:00
20min 58s
def started working 2022-11-05 14:32:30 +02:00
def stopped working 2022-11-05 15:17:22 +02:00
44min 52s
def started working 2022-11-05 15:17:23 +02:00
def stopped working 2022-11-05 15:17:34 +02:00
11s
def started working 2022-11-06 13:40:15 +02:00
def stopped working 2022-11-06 14:23:56 +02:00
43min 41s
def started working 2022-11-07 14:07:53 +02:00
def stopped working 2022-11-07 14:52:54 +02:00
45min 1s
def started working 2022-11-07 15:21:10 +02:00
def stopped working 2022-11-07 15:39:05 +02:00
17min 55s
def added 1 commit 2022-11-07 16:09:41 +02:00
def added spent time 2022-11-07 16:10:43 +02:00
8min
def started working 2022-11-08 01:14:53 +02:00
def stopped working 2022-11-08 01:44:10 +02:00
29min 17s
def started working 2022-11-08 15:22:43 +02:00
def stopped working 2022-11-08 15:46:23 +02:00
23min 40s
def added spent time 2022-11-08 16:56:48 +02:00
30min
def started working 2022-11-08 17:13:12 +02:00
def stopped working 2022-11-08 17:59:10 +02:00
45min 58s
def started working 2022-11-08 18:46:52 +02:00
def cancelled time tracking 2022-11-08 22:09:48 +02:00
def added spent time 2022-11-08 22:17:29 +02:00
2h
def started working 2022-11-09 01:57:30 +02:00
def stopped working 2022-11-09 02:54:30 +02:00
57min
def added spent time 2022-11-09 03:33:44 +02:00
30min
def added spent time 2022-11-09 03:54:38 +02:00
20min
def started working 2022-11-09 23:34:36 +02:00
def stopped working 2022-11-10 01:39:18 +02:00
2h 4min 42s
def added 1 commit 2022-11-10 01:42:42 +02:00
def started working 2022-11-11 11:59:43 +02:00
def stopped working 2022-11-11 12:15:11 +02:00
15min 28s
def added spent time 2022-11-11 13:14:08 +02:00
15min
def started working 2022-11-11 13:14:12 +02:00
def stopped working 2022-11-11 15:16:03 +02:00
2h 1min 51s
def added spent time 2022-11-12 21:47:42 +02:00
10min
def started working 2022-11-12 21:47:44 +02:00
def stopped working 2022-11-12 21:55:03 +02:00
7min 19s
def started working 2022-11-12 22:13:52 +02:00
def stopped working 2022-11-12 22:33:46 +02:00
19min 54s
def added spent time 2022-11-13 02:48:49 +02:00
30min
def started working 2022-11-13 15:03:44 +02:00
def stopped working 2022-11-13 15:58:58 +02:00
55min 14s
def added spent time 2022-11-14 03:02:31 +02:00
25min
def started working 2022-11-14 03:02:35 +02:00
def stopped working 2022-11-14 03:24:36 +02:00
22min 1s
def started working 2022-11-14 03:29:16 +02:00
def added 1 commit 2022-11-14 04:21:19 +02:00
def requested review from inex 2022-11-14 04:21:35 +02:00
def stopped working 2022-11-14 04:26:29 +02:00
57min 13s
inex was assigned by def 2022-11-14 04:26:44 +02:00
def added 1 commit 2022-11-15 14:21:39 +02:00
def added 1 commit 2022-11-15 18:29:38 +02:00
inex added 1 commit 2022-11-16 12:25:35 +02:00

Taking this PR over.

Taking this PR over.
inex refused to review 2022-11-16 12:27:18 +02:00
inex added 1 commit 2022-11-16 12:58:45 +02:00
inex added 1 commit 2022-11-16 19:08:12 +02:00

Ok, here's the final review.

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.

  • You didn't properly work with datetimes. Serialization format is incosistent. Test did not cover different datetime formats on input.
  • A lot of KeyErrors. Shipping this code is dangerous. Your tests did not cover this. Tests MUST cover basic edge cases on input data. Like:
    • There is no data (there is no key in JSON)
    • Data is null (there is a key, but value is null)
    • Data is in invalid format
  • Opening file with write permissions when you only read data. This has consequences. When you open file for read-only, we use shared file lock. When we open file for writing, we use exclusive lock. You SHOULD learn about how different file locks work on Unix-like systems, and MUST understand how we use them in our project.
  • Exceptions SHOULD NOT have Error in their name. Errors and Exceptions are semantically different objects.
  • You SHOULD have a testing strategy. You had two similar objects: recovery key and new device key. You didn't test them in a similar manner. Because of that, your tests found error in one object, but not in the another. Keep a clear structure of your tests. Group test cases. Split them into different files if needed for clarity.
  • You MUST use Conventional Commits in every commit.
Ok, here's the final review. > The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119. - You didn't properly work with `datetime`s. Serialization format is incosistent. Test did not cover different datetime formats on input. - A lot of `KeyError`s. Shipping this code is **dangerous**. Your tests did not cover this. Tests MUST cover basic edge cases on input data. Like: - There is no data (there is no key in JSON) - Data is null (there is a key, but value is null) - Data is in invalid format - Opening file with write permissions when you only read data. **This has consequences**. When you open file for read-only, we use shared file lock. When we open file for writing, we use exclusive lock. You SHOULD learn about how different file locks work on Unix-like systems, and MUST understand how we use them in our project. - Exceptions SHOULD NOT have `Error` in their name. `Error`s and `Exception`s are semantically different objects. - You SHOULD have a testing strategy. You had two similar objects: recovery key and new device key. You didn't test them in a similar manner. Because of that, your tests found error in one object, but not in the another. Keep a clear structure of your tests. Group test cases. Split them into different files if needed for clarity. - You MUST use Conventional Commits in every commit.
inex changed title from WIP: refactor(repository): Tokens repository JSON backend to refactor(repository): Tokens repository JSON backend 2022-11-16 19:11:15 +02:00
inex merged commit e130d37033 into master 2022-11-16 19:12:39 +02:00
inex deleted branch def/jobs-repo 2022-11-16 19:12:39 +02:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
3 Participants
Notifications
Total Time Spent: 18 hours 47 minutes
def
18 hours 47 minutes
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

2022-11-08

Dependencies

No dependencies set.

Reference: SelfPrivacy/selfprivacy-rest-api#18
There is no content yet.