#129: Promote User to Artist #204

xiomaraR · 2024-08-30T05:58:24Z

Description

This pull request introduces a new service method to upgrade user accounts from basic users to artist accounts and a corresponding endpoint to facilitate this functionality.

Commit type

feat: Indicates the addition of a new feature or functionality to the project.

Issue

#129

Solution

Service

The upgrade_user_to_artist method has been implemented to handle the user account upgrade process. The function performs the following actions:

Ensures the provided username and JWT token are valid.
Ensure the user is valid
Fetches the user's data from the user_repository
Creates a new artist entry in the artist_repository using the retrieved user data. This includes copying the user's playlists, playback history, and other relevant information
Modifies the user's role in the user_repository to "artist"
Creates a new JWT token with the updated "artist" role
Includes error handling and logging
Returns the new token to the caller

Endpoint

The upgrade_to_artist endpoint has been added to facilitate upgrading a user to an artist via an HTTP PATCH request. It performs the following:

Accepts the username and JWT token in the request
Calls the upgrade_user_to_artist method to process the upgrade
Returns the new JWT token in the response with a 200 OK status code.
Handles various exceptions to provide appropriate HTTP responses

Tests

Tests have been added for the upgrade_user_to_artist method and the new endpoint:

Tests for successful user upgrades
Tests for user not found during upgrade

Proposed Changes

user_service.py: Added the upgrade_user_to_artist function.
user_controller.py: Added the PATCH /users/{name}/upgrade_to_artist endpoint.
artist_repository.py: Added the create_artist_from_user function to handle artist creation from existing user data.
api_test_user.py: Added upgrade_to_artist client endpoint for testing requests
test_user.py: Added tests for the upgrade_user_to_artist function and upgrade_to_artist endpoint

Potential Impact

Users will have the ability to upgrade to artist status, which affects user management and permissions

Screenshots

N/A

Additional Tasks

The current implementation duplicates user data in both the user and artist collections. Needs further consideration and alternate strategies.

Assigned

@AntonioMrtz

xiomaraR · 2024-08-30T06:02:06Z

Hey! I've been having issues with my python versions that I need to look into. I wasn't able to get pytest to work for me, so I didn't run the tests. Can you let me know if there's anything I should change or add? Thanks :)

Backend/app/spotify_electron/user/user/user_service.py

Backend/app/spotify_electron/user/user_controller.py

Backend/tests/test_API/api_test_user.py

Backend/tests/test__user.py

AntonioMrtz · 2024-08-30T10:40:16Z

@xiomaraR From this summer we're using an updated Git convention as seen here. Branch name is OK because it was previous to the update but I think we can follow the convention for PR naming just to get used to it. I would suggest getting a look into the new Git Convention for following contributions :).
Also commit messages should include the prefix (feat,fix...) as shown here. If you know how to ammend and force push the commit message I would suggest you to do it. If its hard for you just consider it for future commits.

AntonioMrtz · 2024-08-30T10:51:38Z

Hey! I've been having issues with my python versions that I need to look into. I wasn't able to get pytest to work for me, so I didn't run the tests. Can you let me know if there's anything I should change or add? Thanks :)

Hi, I checked the pipelines and the new test is failing. Python 3.11 is recommended and previous versions could make the app not to run because of StrEnum type annotations that werent supported before.

For running backend tests I wrote this docs. If the issue still persist tell me what error you're having so I can help you. Take a look at the docs and let me know if the problem was covered there :)

Backend/app/spotify_electron/user/user/user_service.py

…atch endpoint

xiomaraR · 2024-08-31T21:19:53Z

@AntonioMrtz Please let me know if I did too much and if there is a simpler solution. I couldn't figure out how to delete the user after passing the user into create_artist without any errors. So I thought maybe updating roles and transferring the user_data to the artist collection could work for this purpose

AntonioMrtz · 2024-09-01T16:34:05Z

Hi @xiomaraR , first of all thanks for all the work you put into solving this issue.

Formatting

This pipeline is failing because there's files that would be reformatted if ruff is used. So we could begin runing python -m ruff format on the codebase, as seen in the output only only file should be reformatted. I think pre-commit should have also get the formatting error before commiting, I don't know if you have set it up correctly.

Overall solution

Probably I wasn't clear enough on my comments when I requested changes. The code looked good in the previous state and only required some minor changes for adjusting style and some minor docstring that weren't completly accurate.

As said in the requested changes, this method will be called by user admins (it doesn't exists yet, it's a future task) so we can freely delete the user and create an artist and JWT treatment can be basicly excluded. With Admin user creation upgrade method will be moved into admin_service and we will have to check if user is admin, include a comment with TODO in the code for making this change in the future. This pseudo represents the overall schema, all the methods already exists:

def upgrade(user):

	validate_user_parameter(user)
        validate_user_exists(user)
	
	user_data = get_user(user)
	delete_user(user)
	
	create_artist(user_data) // Use existing create artist method
	repo.upgrade_artists_fields_from_user(user_data)

Making atomic operations in database is not yet implemented so we can skip that part for the moment.

Since User and Artist only differ on uploaded_songs attribute all the properties can be exported without losing info and breaking soft links like Playlist owners and so.

The problem lives in adding attributes that create_artist method doesn't accept. We can bypass that with repo.upgrade_artists_fields_from_user(user_data).

If you have any doubts or questions feel free to let me know. :)

AntonioMrtz · 2024-09-01T08:41:16Z

Backend/app/spotify_electron/user/artist/artist_repository.py

@@ -90,6 +91,41 @@ def create_artist(name: str, photo: str, password: bytes, current_date: str) ->
        artist_repository_logger.info(f"Artist added to repository: {artist}")


+def create_artist_from_user(user_data: UserDAO) -> None:


I think we can use the create_artist method from artist repo and upgrade the missing fields with a custom method

AntonioMrtz · 2024-09-01T08:45:53Z

Backend/app/spotify_electron/user/user/user_service.py

+    Raises:
+        UserNotFoundException: if the user does not exist
+        UserServiceException: unexpected error while upgrading user to artist
+        UserBadNameException: if the user name is invalid


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
UserNoutFoundException -> Not found -> 404
Repository,Service,Exceptions -> Internal Server Error -> 500

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.

AntonioMrtz · 2024-09-01T08:48:59Z

Backend/app/spotify_electron/user/user/user_service.py

+        }
+        new_token = auth_service.create_access_token(new_token_data)
+
+    except UserBadNameException as exception:


Here the order is correct, 400, 404, 500 👍

AntonioMrtz · 2024-09-01T08:51:40Z

Backend/app/spotify_electron/user/user_controller.py

+            status_code=HTTP_404_NOT_FOUND,
+            content=PropertiesMessagesManager.userNotFound,
+        )
+    except UserUnauthorizedException:


Is there any methods inside service that can throw this exception? I use it for users trying to create a song and so

AntonioMrtz · 2024-09-01T08:59:28Z

Backend/tests/test__user.py

@@ -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):


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 get_all_users method anymore we can call service method get_user from user_service and check that it throws UserNotFoundException.

AntonioMrtz · 2024-09-01T09:05:24Z

Backend/app/spotify_electron/user/user_controller.py

+    """
+    try:
+        new_token = user_service.upgrade_user_to_artist(name, token)
+        response_data = {"token": new_token}


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:

User requests to be upgraded to artist

Admin checks requests

Admin run this endpoint for upgrading user to artist

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.

AntonioMrtz · 2024-09-01T09:05:58Z

Backend/app/spotify_electron/user/user_controller.py

+        return Response(
+            content=response_json,
+            media_type="application/json",
+            status_code=HTTP_200_OK,


If its a patch method I would return a 204 code for having consistency between other patch methids.

AntonioMrtz · 2024-09-01T09:09:18Z

Backend/app/spotify_electron/user/user/user_repository.py

        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:


I don't think we will need this method anymore

AntonioMrtz · 2024-09-01T09:10:19Z

Backend/app/spotify_electron/user/user/user_service.py

+    try:
+        base_user_service.validate_user_name_parameter(user_name)
+        validate_user_exists(user_name)
+        auth_service.validate_jwt_user_matches_user(token, user_name)


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

AntonioMrtz · 2024-09-27T11:04:22Z

Hi, there's no need to hurry. I only wanted to know if you need help with anything. Feel free to ask :)

xiomaraR requested a review from AntonioMrtz as a code owner August 30, 2024 05:58

AntonioMrtz requested changes Aug 30, 2024

View reviewed changes

AntonioMrtz linked an issue Aug 30, 2024 that may be closed by this pull request

Promote User to Artist #129

Open

xiomaraR closed this Aug 30, 2024

xiomaraR deleted the feat/create-patch-endpoint-promote-user branch August 30, 2024 20:32

xiomaraR restored the feat/create-patch-endpoint-promote-user branch August 30, 2024 20:37

xiomaraR reopened this Aug 30, 2024

AntonioMrtz changed the title ~~Promote User to Artist #129~~ #129: Promote User to Artist Aug 31, 2024

AntonioMrtz requested changes Aug 31, 2024

View reviewed changes

Backend/app/spotify_electron/user/user/user_service.py Show resolved Hide resolved

feat: create user service to upgrade user to artist account and add p…

9dfafe7

…atch endpoint

xiomaraR force-pushed the feat/create-patch-endpoint-promote-user branch from 1c1800f to 9dfafe7 Compare August 31, 2024 20:53

AntonioMrtz requested changes Sep 1, 2024

View reviewed changes

AntonioMrtz force-pushed the master branch from 1dcca58 to 301f3ae Compare September 20, 2024 18:00

AntonioMrtz assigned xiomaraR Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#129: Promote User to Artist #204

#129: Promote User to Artist #204

xiomaraR commented Aug 30, 2024 •

edited

Loading

xiomaraR commented Aug 30, 2024

AntonioMrtz commented Aug 30, 2024 •

edited

Loading

AntonioMrtz commented Aug 30, 2024

xiomaraR commented Aug 31, 2024

AntonioMrtz commented Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz Sep 1, 2024

AntonioMrtz commented Sep 27, 2024

		@@ -90,6 +91,41 @@ def create_artist(name: str, photo: str, password: bytes, current_date: str) ->
		artist_repository_logger.info(f"Artist added to repository: {artist}")


		def create_artist_from_user(user_data: UserDAO) -> None:

		@@ -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):

#129: Promote User to Artist #204

Are you sure you want to change the base?

#129: Promote User to Artist #204

Conversation

xiomaraR commented Aug 30, 2024 • edited Loading

Description

Commit type

Issue

Solution

Service

Endpoint

Tests

Proposed Changes

Potential Impact

Screenshots

Additional Tasks

Assigned

xiomaraR commented Aug 30, 2024

AntonioMrtz commented Aug 30, 2024 • edited Loading

AntonioMrtz commented Aug 30, 2024

xiomaraR commented Aug 31, 2024

AntonioMrtz commented Sep 1, 2024

Formatting

Overall solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonioMrtz commented Sep 27, 2024

xiomaraR commented Aug 30, 2024 •

edited

Loading

AntonioMrtz commented Aug 30, 2024 •

edited

Loading