Python (Flask) - User registration system

Question

I am working on a simple CRUD app as a personal project using Flask. I am currently working on the user service and I just finished the registration process. I am also trying to use as less as libraries as I could, this is why you won't see any ORM or Flask-wtf (for learning purposes).

• models.py: contains 1 class - User. The User class is just a representation of a User object, there is no method present in this class expect a __str__ method.

• services.py: contains 1 class - UserService. No constructor or class variables. The UserService is use to interact with the User such as create a new user, update a user parameter or delete a user. Most of the UserService methods takes a User as a parameter expect the method to create a new User, add_user()

• repo.py: contains 1 class - UserRepository. As I am not using any ORM, I needed a way to interact with the database to add, delete or update rows in the User table. I could have directly make the queries on the User service, but I think I will actually broke the separation of concerns;

• Route.py: Once the user fill the register.html registration form, the form is sent to the /new-user route.

I am wondering about multiple things:

• I read that commit to the repository using sqlite take a decent amount of resources. I do have a save method on my UserRepository class that just do a conn.commit(). But do I have to call this method each time I am creating a new user or like it should be call like after creating x new Users or x new db-operations?

• I am converting my active variable from Boolean to Integer (1 or 0) because this is how it is set in my User table on mySQL db as tinyint(1) DEFAULT '1'. Should I also use 1/0 on my code or keep it as True/False and convert from Boolean to Integer in my UserRepository class?

• If you have any other red flag or design issues that I should not do, let me know :)

Code bellow:

user/routes.py

user = Blueprint('user', __name__, template_folder='templates')


@user.route('/register')
def login():
    return render_template('register.html')

# register new user
@user.route('/new-user',methods = ['POST'])
def register_user():
    # gather form data
    form_email = request.form.get('email')
    form_password = request.form.get('psw')

    # register_user() will return a User object
    new_user = UserService().register_user(form_email, form_password)

    user_repository = UserRepository(conn, 'users')
    user_repository.add_user(new_user)
    user_repository.save()
    # will probably change the return and add try-catch? 
    return "ok"

models.py (User class)

class User():
    
    def __init__(self, email, password, registration_date, 
        active, sign_in_count, current_sign_in_on, last_sign_in_on):

        self.email = email
        self.password = password
        self.registration_date = registration_date
        self.active = active
    
        # Activity tracking
        self.sign_in_count = sign_in_count
        self.current_sign_in_on = current_sign_in_on
        self.last_sign_in_on = last_sign_in_on
    
    def __str__(self):
        user_attributes = vars(self)
        return (', '.join("%s: %s" % item for item in user_attributes.items()))

services.py (UserService class)

class UserService():
    def register_user(self,
                      email,
                      password):

        # sign-in count is 1 since it is a new user
        sign_in_count = 1

        # today dates for registration date, current sign-in date and last sign-in date since it's a new user
        today_date = datetime.today().strftime('%Y-%m-%d')

        active = True
        new_user = User(email, password, today_date, active,
                        sign_in_count, today_date, today_date)
        return new_user

    def desactivate_user(self, User):
        if User.active == False:
            print(f"User {User.email} is already inactive")
        User.active = False

    def reactive_user(self, User):
        if User.active == True:
            print(f"User {User.email} is already active")
        User.active = True

    def is_active(self, User):
        return User.is_active

    def update_activity_tracking(self, User, ip_address):
        User.sign_in_count += 1
        User.last_sign_in_on = User.current_sign_in_on
        User.current_sign_in_on = datetime.datetime.now()

    def update_password(self, User, new_password):
        User.password = get_hashed_password(new_password)

repo.py (UserRepository class)

class UserRepository():
    def __init__(self, conn, table):
        self.conn = conn
        self.table = table  
    
    
    def add_user(self, User):
        sql = "INSERT INTO users (email, password, is_active, sign_in_count, current_sign_in_on, last_sign_in_on) VALUES (%s, %s, %s, %s, %s, %s)"
        cursor = self.conn.cursor()
        # the is_active column in the DB is a tinyint(1). True = 1 and False = 0
        if User.active == True:
            is_active = 1
        is_active = 0
        cursor.execute(sql, ( User.email, User.password, is_active, User.sign_in_count, User.current_sign_in_on, User.last_sign_in_on))
        resp = cursor.fetchall()
        return resp
    
    def delete_user(self):
        return ""
    
    def get_user(self):
        return ""
    
    def save(self):
        self.conn.commit()

Answer 1

Opinions always differ on matters like this, but your UserService class seems like a severe case of over-engineering.

First, three of the methods seem to offer nothing very substantive: desactivate_user() and reactive_user() just set boolean attributes on a User instance; and is_active() just returns an attribute. I would encourage you to simplify things: just operate on the user instance directly.

Second, the update_activity_tracking() does more stuff to the user instance, but there's no obvious reason not to move the method into the User class as well.

Third, the update_password() is a good example of a situation where you could use a property, because you need to perform some calculations before storing the attribute. Here's the basic pattern:

class User:

    def __init__(self, password):
        # Initialize an under-the-hood attribute.
        self._password = None

        # Call the setter.
        self.password = password

    @property
    def password(self):
        # The getter just returns it.
        return self._password

    @property.setter
    def password(self, new_password):
        # The setter does the needed calculation.
        self._password = get_hashed_password(new_password)

Fourth, the register_user() method is a natural fit for a classmethod in the User class:

class User:

    @classmethod
    def register_user(cls, email, password):
        # Python allows you to call functions in keyword style even if the
        # function defines arguments as required positionals. You can use
        # this feature to simplify the code (eliminate some temporary
        # variables in this case) while still preserving code readability.
        today = datetime.today().strftime('%Y-%m-%d')
        return cls(
            email,
            password,
            registration_date = today,
            active = 1,
            sign_in_count = 2,
            current_sign_in_on = today,
            last_sign_in_on = today,
        )

The User.__init__() method has a large number of required parameters. Perhaps your use case demands that, but you might give some thought to which of the attributes are truly required in all usages. For example, the method could default to using registration_date if the two sign-on dates are not provided (as we do above in register_user()). Regardless of that decision, if testing and debugging make direct interaction with __init__() tedious, you can add more conveniences to the class, again using the @classmethod mechanism.

Managing user passwords is tricky business, at least in the real world. It's good that you store a hashed password rather than the plaintext, but I would not include even the hashed password attribute in any kind of printing, logging, etc. Adjust your User.__str__() method accordingly.

In UserRepository.add_user() you can simplify things. If you are testing for truth, just do it directly (if user.active) rather than indirectly (if user.active == True). Also, bool values in Python are a subclass of int (for example, 199 + True == 200), so you can delete the conditional logic entirely and just do something like this:

def add_user(self, user):
    # Long lines can be made more readable by formatting
    # them in the style of pretty-printed JSON. Notice how
    # easy this style is to read, scan visually, and edit flexibly.
    sql = '''
        INSERT INTO users (
            email,
            password,
            is_active,
            sign_in_count,
            current_sign_in_on,
            last_sign_in_on
        )
        VALUES (%s, %s, %s, %s, %s, %s)
    '''.strip()
    cursor = self.conn.cursor()
    cursor.execute(
        sql,
        (
            user.email,
            user.password,
            int(user.active),
            user.sign_in_count,
            user.current_sign_in_on,
            user.last_sign_in_on,
        )
    )
    return cursor.fetchall()

Your capitalization of the User variable within various methods is inconsistent both with your own usages and with the norms in the wider Python community: FooBar for class names and foo_bar for instance variables is the most common approach.

DB operations can fail. If this is just an early learning exercise, you can safely ignore the problem, but if your goal is to make the application more robust, the operations should be wrapped in try-except structures, as hinted at in your code comments.

In my experience, web applications like this will have various classes representing the entities of interest (User, Book, Author, whatever). And several of those classes will need DB operations: create, read, update, delete, etc. And those DB operations will be fairly general, in the sense that if you write a good method to update one kind of instance (eg, User), it can often be made to do the same thing for other kinds of instances, provided that the substantive classes (User, Book, etc) have attributes, properties, or methods to deliver the information (things like table name and a list of (COL_NAME, ATTRIBUTE_VALUE) tuples) that a general-purpose DB method needs in order to perform its operation. All of which is to say that UserRepository might be too narrow of a focus. Instead, think about whether and how you might convert that class into just Repository.

If this web application is intended to be a long-running application, you'll want to wire it up to use the logging framework at some point (rather than just printing stuff), but that can come whenever you are ready to learn about it.

Regarding your question about frequency of saving DB changes, don't worry about it or overthink it. Build your application with reasonable Python code -- you are off to a good start -- and if your application ever needs to achieve higher scale, solve the problem when it is clearly visible on the horizon. It's often easier to change DBs, scale outward (more running instances of your application in different processes or on different servers), or alter the data schema than it is to write tricky "save every N operations" code.