Skip to content

Add some comments #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions accounts/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def create_account(request):
email = req_data['email']
password = req_data['password']
except KeyError as e:
# One might argue that this response is a Presenter by itself
# But having the flexibility on mistyping "success" for example is a bit of a deal breaker for me. I'd rather have something which received the 3 params (success, message and status) and generated the messages
return JsonResponse(
{
'success': False,
Expand Down
3 changes: 3 additions & 0 deletions adapters/django_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class DjangoStorage(Storage):
"""Adapter to use Django ORM as a storage backend."""

def create_user(self, user, password):
# not injecting repo makes it less explicit when testing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean what you call repo_provider in your code? So the User.objects here?

"""Create user entity.

Because Django provides password hashing functionality that we want to use, we perform this
Expand All @@ -22,6 +23,7 @@ def create_user(self, user, password):
django_user = accounts_models.User.objects.from_entity(user)
django_user.set_password(password)
django_user.save()
# nizar: unnecessary coupling with the return from the repo;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I prefer a function that takes a model as an argument and returns the entity.

return django_user.to_entity()

def save_board(self, board):
Expand Down Expand Up @@ -98,6 +100,7 @@ def delete_board_user(self, user_id, board_id):
raise self.DoesNotExist('User {} is not joined to board {}'.format(user_id, board_id))

django_board_user.delete()
# nizar: interesting choice here, not sure why it returns this kind of object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this should return an entity to be consistent right?

return django_board_user.asdict()

def delete_board(self, id):
Expand Down
1 change: 1 addition & 0 deletions adapters/memory_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from .storage import Storage

# nizar: very cool exercise for leveraging the advantages of having a common interface for a repo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this makes it clear why you want to abstract the repository so you can switch between django storage and memory storage easily and test the business rules.

class MemoryStorage(Storage):
"""Adapter to use system memory as a storage backend."""

Expand Down
3 changes: 2 additions & 1 deletion notes/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
behavior to actions performaned by users. The examples we show are decorators for logging
transation data and checking permissions on a per-action basis.
"""
# This is a pretty cool concept, never had heard of it.

from topsy.action_decorators import permission, log
from .use_cases import NoteUseCases


class NoteActions():
class NoteActions:
def __init__(self, storage, logging):
self.use_cases = NoteUseCases(storage)
self.logging = logging
Expand Down
6 changes: 6 additions & 0 deletions notes/tests/test_use_cases.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
As much as possible we will test use cases with MemoryStorage to enforce independence between the
use cases and the Django ORM.
"""
# Ohhhhhh, this was surprising! So the whole Memory Storage was not to make a point on showing the advantages on switching the repo in a easy way. It was to actually be able to use it in these tests.

# The author mixes many types of test when testing the Use Cases/Interactors. He definitely takes advantage of the in memory storage, but we end up having a weird mixture of tests: we have component tests(which is awesome and the perfect use case for the in memory storage), integration tests that don't actually integrate and system tests which don't actually test the system.

# Martin Fowler has a very cool diagram that defines the frontiers between the different kind of tests very well: https://martinfowler.com/articles/microservice-testing/meta-image.png


import unittest

Expand Down