Feature/user registration schema changes #15

caiola · 2023-05-22T12:29:54Z

No description provided.

sergiodenboer

This MR is too big for an effective review.
We should split into smaller chunks.
Feedback loop is going to be too big.

sergiodenboer · 2023-05-22T22:42:38Z

README.md

@@ -240,7 +240,35 @@ Add a new database migration
 ```
 flask db migrate -m "Schema"
 flask db migrate -m "Add unique constraint to Ad model"
+flask db migrate -m "Add account changes"


Why this log?

Not a log, I copy/pasted and left it there 👯

sergiodenboer · 2023-05-22T22:46:43Z

README.md

+cd backend
+docker build -t img-local-poetry -f backend/dockerfile-apiserver-local ./backend
+docker run -it --rm -v %cd%:/app -w /app img-local-poetry /bin/bash -c "poetry lock"
+


I do a little differently. I would say the added complexity is not needed.
There is a docker-compose file just for development.
justo do:
docker-compose exec app bash

Once inside the container do all the needed commands.

Env file looks a bit like this.

ENVIRONMENT=DEV MYSQL_ROOT_PASSWORD=passowrd MYSQL_PASSWORD=12345 MYSQL_USER=vinhos MYSQL_DATABASE=vinhos_dev MYSQL_HOST=db MYSQL_PORT=3306 TIMEZONE=Europe/Lisbon

This was to solve the error of a poetry dependency "poetry 'HTTPResponse' object has no attribute 'strict'".

It is a recent issue and a pain to solve.
python-poetry/poetry#7936

With docker compose, you caanot run the command "poetry lock" because the container "apiserver" does not start

sergiodenboer · 2023-05-22T22:47:27Z

backend/api/models/abc.py

@@ -69,11 +69,12 @@ def _to_dict(self):
            for column in inspect(self.__class__).attrs
        }

-    def save(self):
+    def save(self, refresh=False):


why the option refresh=False? To retrieve the schema?

yes, after insert, you want to get the ID of the record that was inserted.

sergiodenboer · 2023-05-22T22:48:40Z

backend/api/models/accounts.py


-    account_name = db.Column(db.String(50), unique=True, nullable=False)
+    account_name = db.Column(db.String(60), nullable=True, comment="Account can have any name, it is an internal reference")


Why is the field nullable?

It will be managed by store name, this will be a internal reference field.

sergiodenboer · 2023-05-22T22:49:49Z

backend/api/models/accounts.py

@@ -16,10 +16,11 @@ class Account(db.Model, BaseModel, metaclass=MetaBaseModel):

    id = db.Column(db.BigInteger(), primary_key=True, nullable=False)
    status_id = db.Column(db.Integer(), default=StatusType.NEW, nullable=False, comment="Status id")
-    address_id = db.Column(db.BigInteger(), nullable=False, comment="Address id")
+    address_id = db.Column(db.BigInteger(), nullable=True, comment="Address id")


Isn't the adress required?
Why is address in a different schema? This will result in a one to many relation. On the other hand ther will be the possibility of multiple adresses having the same account. Is this what we want?

Yes, this will be like Ebay, we have the billing address, delivery address, dispatch address if necessary and store addresses.

One account can have several stores.
One account will be able to manage several addresses and each address is associated with an entity.

sergiodenboer · 2023-05-22T23:09:57Z

backend/api/repositories/users.py

+                                       "invalid": "email-invalid-type",
+                                       "type": "email-invalid-must-be-string"})
+
+    # account_id = fields.Int(required=True)


Commented please remove.

sergiodenboer · 2023-05-22T23:10:26Z

backend/api/repositories/users.py

@@ -31,39 +45,45 @@ def update(user: User, **kwargs) -> User:
    return user.save()


SAving on get_by. This is probably wrong.

sergiodenboer · 2023-05-22T23:11:38Z

backend/api/repositories/users.py

+        user_result = user.save(refresh=True)
+    except (IntegrityError, PyMySQLIntegrityError) as e:
+        user_result = None
+        abort(400, _("A user with this email already exists. Please use a different email."))


This is wrong. The error might not error due to email only. :D

Email is unique for login purposes to identify the user.

sergiodenboer · 2023-05-22T23:11:58Z

backend/api/repositories/users.py

+    except (IntegrityError, PyMySQLIntegrityError) as e:
+        user_result = None
+        abort(400, _("A user with this email already exists. Please use a different email."))
+    except Exception as e:


We should not catch on broad exceptions.

This is a special case because of the password_hash.
I dont want to log anything in this part.

sergiodenboer · 2023-05-22T23:12:20Z

backend/api/resources/accounts/resources.py

@@ -16,34 +17,35 @@ def post(self):
        resource_parser = reqparse.RequestParser(trim=True, bundle_errors=True)

        # Add arguments
+        # resource_parser.add_argument(


remove comments.

sergiodenboer

This MR is too big for an effective review.
We should split into smaller chunks.
Feedback loop is going to be too big.

sergiodenboer

This MR is too big for an effective review.
We should split into smaller chunks.
Feedback loop is going to be too big.

caiola added 4 commits May 22, 2023 13:22

Schema changes

330327d

SQL - Account changes

79a0231

SQL - Add user fields

3e4fb2d

DVL - User registration, creates a new store

fcced8d

caiola self-assigned this May 22, 2023

caiola added the enhancement New feature or request label May 22, 2023

caiola added this to the v1.0 milestone May 22, 2023

caiola linked an issue May 22, 2023 that may be closed by this pull request

User registration #13

Closed

caiola added 2 commits May 22, 2023 20:33

SQL - Change store fields nullable to true

a83ee58

DVL - Add .abc feature to refresh data (e.g. allows to get after save)

bb37df4

sergiodenboer reviewed May 22, 2023

View reviewed changes

sergiodenboer requested changes May 22, 2023

View reviewed changes

SQL - Change store field account_id to remove the default value

0eb863a

caiola merged commit e7474a2 into feature/user-registration May 23, 2023

caiola deleted the feature/user-registration-schema-changes branch May 23, 2023 10:03

caiola temporarily deployed to dev May 23, 2023 15:36 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/user registration schema changes #15

Feature/user registration schema changes #15

caiola commented May 22, 2023

sergiodenboer left a comment

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer May 22, 2023

caiola May 23, 2023

sergiodenboer left a comment

sergiodenboer left a comment


		account_name = db.Column(db.String(50), unique=True, nullable=False)
		account_name = db.Column(db.String(60), nullable=True, comment="Account can have any name, it is an internal reference")

		@@ -31,39 +45,45 @@ def update(user: User, **kwargs) -> User:
		return user.save()

Feature/user registration schema changes #15

Feature/user registration schema changes #15

Conversation

caiola commented May 22, 2023

sergiodenboer left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiodenboer left a comment

Choose a reason for hiding this comment

sergiodenboer left a comment

Choose a reason for hiding this comment