-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
#129: Promote User to Artist #204
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,7 +78,36 @@ def create_user(name: str, photo: str, password: bytes, current_date: str) -> No | |
user_repository_logger.exception(f"Error inserting User {user} in database") | ||
raise UserRepositoryException from exception | ||
except (UserRepositoryException, Exception) as exception: | ||
user_repository_logger.exception(f"Unexpected error inserting user {user} in database") | ||
user_repository_logger.exception( | ||
f"Unexpected error inserting user {user} in database" | ||
) | ||
raise UserRepositoryException from exception | ||
else: | ||
user_repository_logger.info(f"User added to repository: {user}") | ||
|
||
|
||
def update_user_role(user_name: str, new_role: str) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we will need this method anymore |
||
"""Update user role | ||
|
||
Args: | ||
name (str): user name | ||
new_role (str): user role | ||
|
||
Raises: | ||
UserNotFoundException: user was not found | ||
UserRepositoryException: unexpected error while updating user role | ||
""" | ||
try: | ||
user_collection = user_collection_provider.get_user_collection() | ||
user = user_collection.find_one({"name": user_name}) | ||
validate_user_exists(user) | ||
user_collection.update_one({"name": user_name}, {"$set": {"role": new_role}}) | ||
except UserNotFoundException as exception: | ||
raise UserNotFoundException from exception | ||
except Exception as exception: | ||
user_repository_logger.exception( | ||
f"Error updating User {user_name} role in database" | ||
) | ||
raise UserRepositoryException from exception | ||
else: | ||
user_repository_logger.info(f"User {user_name} role updated to {new_role}") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,10 +3,12 @@ | |
""" | ||
|
||
import app.auth.auth_service as auth_service | ||
import app.spotify_electron.user.artist.artist_repository as artist_repository | ||
import app.spotify_electron.user.base_user_repository as base_user_repository | ||
import app.spotify_electron.user.providers.user_collection_provider as user_collection_provider | ||
import app.spotify_electron.user.user.user_repository as user_repository | ||
import app.spotify_electron.user.validations.base_user_service_validations as base_user_service | ||
from app.auth.auth_schema import TokenData | ||
from app.logging.logging_constants import LOGGING_USER_SERVICE | ||
from app.logging.logging_schema import SpotifyElectronLogger | ||
from app.spotify_electron.user.user.user_schema import ( | ||
|
@@ -18,6 +20,9 @@ | |
UserServiceException, | ||
get_user_dto_from_dao, | ||
) | ||
from app.spotify_electron.user.validations.base_user_repository_validations import ( | ||
validate_user_exists, | ||
) | ||
from app.spotify_electron.utils.date.date_utils import get_current_iso8601_date | ||
|
||
user_service_logger = SpotifyElectronLogger(LOGGING_USER_SERVICE).getLogger() | ||
|
@@ -187,3 +192,51 @@ def search_by_name(name: str) -> list[UserDTO]: | |
f"Unexpected error in User Service getting items by name {name}" | ||
) | ||
raise UserServiceException from exception | ||
|
||
|
||
# TODO wrap within DB transaction to be treated as atomic operation | ||
def upgrade_user_to_artist(user_name: str, token: TokenData) -> None: | ||
"""Upgrade user account to artist account | ||
|
||
Args: | ||
user_name (str): user name | ||
token (TokenData): token data from user | ||
|
||
Raises: | ||
UserNotFoundException: if the user does not exist | ||
AntonioMrtz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
UserServiceException: unexpected error while upgrading user to artist | ||
UserBadNameException: if the user name is invalid | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually put more general exceptions under concrete exceptions. We also use the HTTP return code that the exception would return as a sorting mechanism. Example: UserBadNameException -> Bad Parameter -> 400 So based on the convention the order should look similar to the fragment above. Also for generating docs in VSCODE we use this extension with the google convention for docstrings. This will generate docstring types and paramter automatically. For Jetbrains I think its built in the editor itself. |
||
""" | ||
try: | ||
base_user_service.validate_user_name_parameter(user_name) | ||
AntonioMrtz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
validate_user_exists(user_name) | ||
auth_service.validate_jwt_user_matches_user(token, user_name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method would be exclusive for admins in the future, I should have give this context before. So checking jwt token is from user is not needed. Put a # TODO comment saying we will need to add user is admin check in the future |
||
user_data = user_repository.get_user(user_name) | ||
artist_repository.create_artist_from_user(user_data) | ||
user_repository.update_user_role(user_name, "artist") | ||
new_token_data = { | ||
"access_token": user_name, | ||
"role": "artist", | ||
"token_type": "bearer", | ||
} | ||
new_token = auth_service.create_access_token(new_token_data) | ||
|
||
except UserBadNameException as exception: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here the order is correct, 400, 404, 500 👍 |
||
user_service_logger.exception(f"Bad User Name Parameter: {user_name}") | ||
raise UserBadNameException from exception | ||
except UserNotFoundException as exception: | ||
user_service_logger.exception(f"User not found: {user_name}") | ||
raise UserNotFoundException from exception | ||
except UserRepositoryException as exception: | ||
user_service_logger.exception( | ||
f"Unexpected error in User Repository upgrading user to artist: {user_name}" | ||
) | ||
raise UserServiceException from exception | ||
except Exception as exception: | ||
user_service_logger.exception( | ||
f"Unexpected error in User Service upgrading user to artist: {user_name}" | ||
) | ||
raise UserServiceException from exception | ||
else: | ||
user_service_logger.info(f"Account {user_name} upgraded to artist successfully") | ||
AntonioMrtz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return new_token |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -165,6 +165,53 @@ def delete_user(name: str) -> Response: | |
) | ||
|
||
|
||
@router.patch("/{name}/upgrade_to_artist") | ||
def upgrade_to_artist( | ||
name: str, token: Annotated[TokenData, Depends(JWTBearer())] | ||
) -> Response: | ||
"""Upgrade user account to artist account | ||
|
||
Args: | ||
name (str): user name | ||
token (TokenData): the jwt token. Defaults to None. | ||
""" | ||
try: | ||
new_token = user_service.upgrade_user_to_artist(name, token) | ||
response_data = {"token": new_token} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's interesting. I didn't think about the JWT credentials. I expect this method to be called for an admin when the user is not logged. The workflow I tought about was:
In this issue we only care about the third point. So the JWT will be given to the user the next time he logs in. |
||
response_json = json_converter_utils.get_json_from_model(response_data) | ||
return Response( | ||
content=response_json, | ||
media_type="application/json", | ||
status_code=HTTP_200_OK, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If its a patch method I would return a 204 code for having consistency between other patch methids. |
||
) | ||
except UserBadNameException: | ||
return Response( | ||
status_code=HTTP_400_BAD_REQUEST, | ||
content=PropertiesMessagesManager.userBadName, | ||
) | ||
except UserNotFoundException: | ||
return Response( | ||
status_code=HTTP_404_NOT_FOUND, | ||
content=PropertiesMessagesManager.userNotFound, | ||
) | ||
except UserUnauthorizedException: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any methods inside service that can throw this exception? I use it for users trying to create a song and so |
||
return Response( | ||
status_code=HTTP_403_FORBIDDEN, | ||
content=PropertiesMessagesManager.userUnauthorized, | ||
) | ||
except BadJWTTokenProvidedException: | ||
return Response( | ||
status_code=HTTP_403_FORBIDDEN, | ||
content=PropertiesMessagesManager.tokenInvalidCredentials, | ||
headers={"WWW-Authenticate": "Bearer"}, | ||
) | ||
except (Exception, UserServiceException): | ||
return Response( | ||
status_code=HTTP_500_INTERNAL_SERVER_ERROR, | ||
content=PropertiesMessagesManager.commonInternalServerError, | ||
) | ||
|
||
|
||
@router.patch("/{name}/playback_history") | ||
def patch_playback_history( | ||
name: str, song_name: str, token: Annotated[TokenData, Depends(JWTBearer())] | ||
|
AntonioMrtz marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,13 @@ | |
import app.spotify_electron.user.base_user_service as base_user_service | ||
import app.spotify_electron.user.user.user_service as user_service | ||
from app.auth.auth_schema import VerifyPasswordException | ||
from tests.test_API.api_test_user import create_user, delete_user, get_user | ||
from tests.test_API.api_test_artist import get_artist | ||
from tests.test_API.api_test_user import ( | ||
create_user, | ||
delete_user, | ||
get_user, | ||
upgrade_to_artist, | ||
) | ||
from tests.test_API.api_token import get_user_jwt_header | ||
|
||
|
||
|
@@ -124,6 +130,59 @@ def test_check_encrypted_password_different(): | |
base_user_service.delete_user(name) | ||
|
||
|
||
def test_upgrade_user_to_artist_correct(clear_test_data_db): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like these tests 😁, good job. For extra style points we could check if the user stills exists on both as artist and as regular user. Since I don't think we have |
||
name = "8232392323623823723" | ||
photo = "https://photo" | ||
password = "hola" | ||
|
||
res_create_user = create_user(name=name, password=password, photo=photo) | ||
assert res_create_user.status_code == HTTP_201_CREATED | ||
|
||
jwt_headers = get_user_jwt_header(username=name, password=password) | ||
|
||
res_get_user = get_user(name=name, headers=jwt_headers) | ||
assert res_get_user.status_code == HTTP_200_OK | ||
assert res_get_user.json()["name"] == name | ||
assert res_get_user.json()["photo"] == photo | ||
|
||
res_upgrade_user = upgrade_to_artist(user_name=name, headers=jwt_headers) | ||
assert res_upgrade_user.status_code == HTTP_200_OK | ||
|
||
new_token_data = res_upgrade_user.json() | ||
assert "token" in new_token_data | ||
new_token = new_token_data["token"] | ||
|
||
decoded_token = auth_service.get_jwt_token_data(new_token) | ||
assert decoded_token.role == "artist" | ||
|
||
res_get_artist = get_artist(name=name, headers=jwt_headers) | ||
assert res_get_artist.status_code == HTTP_200_OK | ||
artist_data = res_get_artist.json() | ||
assert artist_data["name"] == name | ||
assert artist_data["photo"] == photo | ||
|
||
base_user_service.delete_user(name) | ||
|
||
|
||
def test_upgrade_to_artist_user_not_found(): | ||
name = "8232392323623823723" | ||
photo = "https://photo" | ||
password = "hola" | ||
|
||
res_create_user = create_user(name=name, password=password, photo=photo) | ||
assert res_create_user.status_code == HTTP_201_CREATED | ||
|
||
jwt_headers = get_user_jwt_header(username=name, password=password) | ||
res_get_user = get_user(name=name, headers=jwt_headers) | ||
assert res_get_user.status_code == HTTP_200_OK | ||
assert res_get_user.json()["name"] == name | ||
assert res_get_user.json()["photo"] == photo | ||
base_user_service.delete_user(name) | ||
|
||
res_upgrade_user = upgrade_to_artist(user_name=name, headers=jwt_headers) | ||
assert res_upgrade_user.status_code == HTTP_404_NOT_FOUND | ||
|
||
|
||
# executes after all tests | ||
@pytest.fixture() | ||
def clear_test_data_db(): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use the
create_artist
method from artist repo and upgrade the missing fields with a custom method