Restructure and reformat the whole project

[gorkemarslan]

As far as I can audit, I found some issues that need to be done before adding the new features. Otherwise, strong conflicts may appear either now or later on. I am planning to complete all of them as soon as possible, like in a couple of days.

Tasks

• 1. Update accounts app

  • 1.1. Reformat accounts.models.py
    • Refactor Account with Profile as suggested this inactive PR Issue #926 fixed #955. Unfortunately, that PR is unsuccessful. The project won't run with those changes because there is still more untouched stuff related to the Account class and also "account" is present in the comments of the applications. (@brylie could you remove that PR and the issue? I will handle it here and also migrate.)
    • User model is misused. There is a defined class called User and also a variable called User in the file. Further, we shouldn't use User = get_user_model() in the accounts/models.py. User class is present in the same file so it is meaningless to call it from settings.py.
    • Reconsider User and Profile (Account) models. Some fields are redundant, some fields can be added.
  • 1.2. Update views and urls
    • Update login, logout, and register views with URLs. Add unit testing. (issue Improve unit test coverage #952 )
    • Update password activation, profile activation etc views and URLs. Add unit testing.

• 2. Update api app

  • There are still Profile related classes and functions (e.g. ProfileSerializer)

• 3. Move all common helper functions to a new project file named common

  • PathAndRename is defined four times in 3 different files. Move this class to the common folder.
  • Move other common functions such as decorators etc.

• 4. Put static files to the correct place

• 5. Remove unused folders, files, and objects.

  • test_framework folder is not being used now.

• 6. Update settings.py

  • A maintenance is must.

• 7. Reformat the whole project with flake8 and black

I will edit and add new tasks as I detect any issues.

After these changes, the project becomes more convenient to collaborate and contribute.

@brylie, could you assign me to this issue? You can also leave a comment for inactivate issues related to restructuring and reformatting. I will add them to this list.

[AbhijithGanesh]

I can help in restructuring parts of this project but I think it's a behemoth of task to single handedly solve.

[gorkemarslan]

@AbhijithGanesh, your interest in this issue is welcomed, I am grateful to you. 😌 Unfortunately, the changes that I want to make are related to the other parts of the project. (e.g. moving a class to another file, changing names). So, I think, if more than one person works on this issue, conflicts are inevitable and the whole process takes much time and attention. I've already started working on the tasks and try to complete them as soon as possible. I am going to commit 5-6 light changes as I will be assigned to this issue. Then heavy changes will come. Of course, my finished work won't be perfect, then you can fix my faults and the errors that the project still has. As it stands now, changing and updating the project is more difficult.

But, you can work on the parts that aren't related to the backend. For now, you may take responsibility for transferring missing files from the master branch (e.g. static folder). I think @brylie will help you with what can be done.

TL;DR: I'm just concerned with the possible conflicts that will be caused by committing by multiple people.

[AbhijithGanesh]

Points well noted, thanks for putting a word out. Really appreciate the work and efforts mate. I didn't think about the conflicts standpoint. Thank you for the clarification

[brylie]

One additional goal is to deprecate the api app altogether. We will likely need to introduce a new discussions app that contains all code related to Civis, and Threads. Rather than having a dedicated api app, we will have an api.py within the discussions, accounts, and other apps that contain API-related code. The remainder of the code should move to conventional models.py, templates, etc.

[brylie]

@gorkemarslan, I appreciate your vision here. This task is comprehensive 😃

[brylie]

@AbhijithGanesh we are moving away from the SPA front-end architecture. The SPA architecture proved to be overengineering for a project of this size and has introduced a maintenance burden that exceeds our capacity.

Instead, we will use more traditional server-rendered templates from Django with sprinkles of JavaScript (such as using HTMX) where relevant. Please see the related discussions below:

• Deprecate the front-end JavaScript codebase #827
• Refactor backend code to follow Django app structure #83

[gorkemarslan]

@brylie, accounts/authentication.py is also depreciated (except for send_activation_email() function), right? This is related to Task 1.2.

I am planning to keep CBVs and remove FBVs for the authentication system.

[AbhijithGanesh]

@brylie, accounts/authentication.py is also depreciated (except for send_activation_email() function), right? This is related to Task 1.2.

I am planning to keep CBVs and remove FBVs for the authentication system.

Can you explain the terms please.

[brylie]

@gorkemarslan CBVs sound good to me.

If possible, let's deprecate the send_activation_email() function, and default to standard Django account activation flow. Less code is better.

[brylie]

Can you explain the terms please.

Django uses the concept of views to prepare and send data to the end-user, typically in the form of rendered HTML templates but sometimes as JSON or other formats. In the simplest form, a Django view is just a function (called a function-based view, or FBV) that returns an HTTP response with the aforementioned payload. However, Django also lets us define views using Python classes, called class-based views (CBV). CBVs have benefits over FBVs, such as inheritance. Generally, it is preferable to use CBVs when possible.

Further reading:

• Django docs: Introduction to class-based views¶
• Advanced Django Training: Class-based Views

[AbhijithGanesh]

Oh you were discussing about Class based views. Noted and Thanks Brylie.

[gorkemarslan]

@AbhijithGanesh, CBV stands for Class-Based View, FBV stands for Function-Based Views. They are different paradigms on how to render HTML with views.py. CBVs are abstracter than FBVs so they are less prone to errors, easy to maintenance and test, etc. We generally tend to use CBVs because of these reasons.

[gorkemarslan]

@brylie, ok, I'll work on that. What about URLs? I, for example, intent to use login/ instead of accounts/login/ for simpliticy.

[AbhijithGanesh]

@AbhijithGanesh, CBV stands for Class-Based View, FBV stands for Function-Based Views. They are different paradigms on how to render HTML with views.py. CBVs are abstracter than FBVs so they are less prone to errors, easy to maintenance and test, etc. We generally tend to use CBVs because of these reasons.

I understand the benefits of CBV , I just didn't know the expansion. Thank you for the explanation

[brylie]

I, for example, intent to use login/ instead of accounts/login/ for simpliticy.

Sounds good.

[gorkemarslan]

@brylie, I will also remove frontend_views folder, which is the third one that handles views and URLs for the authentication system. In the end, only CBVs will be responsible for all authentication operations.

[gorkemarslan]

@brylie, I have a question.

Account model is defined such that:

class Account(models.Model):
    user = models.ForeignKey(User, on_delete=models.CASCADE)
    first_name = models.CharField(max_length=63, blank=False)
    last_name = models.CharField(max_length=63, blank=False)
    about_me = models.CharField(max_length=511, blank=True)
. . .

In accounts/forms.py:

class AccountRegistrationForm(ModelForm):
. . .
    class Meta:
        model = User
        fields = ("username", "email", "password")
. . .

I think AccountRegistrationForm should include first_name and last_name fields. Notice that blank=False so we want AccountRegistrationForm not to validate the form in the case of empty first and last name fields.

Am I wrong? Which fields do we want the new users to fill? According to your response, I can update the form.

[brylie]

@gorkemarslan the User model is the only model required for registration and authentication purposes.

The name Account is a bit ambiguous, so the Account model should be renamed to Profile. All fields on the Profile should be optional, including first_name and last_name.

Apologies for the confusion. The model renaming and field refactoring are work-in-progress.

[gorkemarslan]

@brylie, I am going to take care renaming Account model, I'm just waiting for the pending PR. I intentionally used Account to specify AccountRegistrationForm so there is no confusion in here. :)

I just realize a point thanks to your reply. I am planning to rename AccountRegistrationForm to ProfileRegistrationForm. Actually, both of them are wrong. UserRegistrationForm is the correct one because the form is used to create a User model object, neither Account nor Profile.

Lastly, If everything's going well, I will tonight commit my changes for the authentication system. (Remove FBVs and related urls, upgrade urls.py to Django 3.2, use CBV, add unit tests for the authentication part.)

Thanks for your reply. Now it is much clear. :)

[brylie]

UserRegistrationForm is the correct one because the form is used to create a User model object, neither Account nor Profile.

Yes. We only need to register a User account so people can log in. Everything in the Account model, soon to be renamed to Profile, is optional. So we need UserRegistrationForm, as you mention, and a ProfileEditForm for users who want to fill in their Profile.

[brylie]

@gorkemarslan I have merged the changes to rename Account to Profile throughout the codebase.

[gorkemarslan]

@brylie, threads app does not have any purpose right now. However, INSTALLED_APPS includes this app. I think we can move the files inside it to the correct place and remove it.

[brylie]

@gorkemarslan sounds good. Keep in mind the idea of a discussions app that will contain civis, threads, and related files.

[gorkemarslan]

Ok, then we shouldn't touch threads for now. Just after we create the discussions app, we can delete threads.

I have two questions:

1. Are we going to customize the User model?

In accounts/models.py two User variables are assigned in that order:

class User(AbstractUser):
    """
    A new custom User model for any functionality needed in the future. Extending AbstractUser
    allows for adding new fields to the user model as needed.
    """

    class Meta:
        db_table = "users"

And

# get custom user model
User = get_user_model()

It is obvious that the first User is overridden by the second one, so the usage of the first one is meaningless. It does not have any effect for the project, so it is redundant.

Moreover, it would be better if we define Profile model such that:

from django.contrib.auth.models import User

class Profile(models.Model):
    user = models.ForeignKey(User, on_delete=models.CASCADE)
    . . .

So if it is Ok and we won't customize the User model, I am going to remove the first and the second code snippets from the project. The third code snippet will be added/updated as shown.

2. I am very skeptical to use User = get_user_model() outside the classes and the functions. So it is defined globally in the various parts of the projects. We import the our project modules that have such a usage, no matter User will be used or not by the importing file, User object is created each time. We can either put it into the related scopes or change User with get_user_model() where it is used, apart from the accounts/models.py file. So, we should call the user model only when we need it.

[AbhijithGanesh]

Once the restructuring is over, can we please have a CD pipeline which can be deployed on heroku? It shall be easier for us to review

[brylie]

Remove this one from accounts/models.py:

# get custom user model
User = get_user_model()

[brylie]

Even though we are not currently customizing the User model, it is important to define a custom User model early in project development, as per the Django documentation.

If you’re starting a new project, it’s highly recommended to set up a custom user model, even if the default User model is sufficient for you.

https://docs.djangoproject.com/en/3.2/topics/auth/customizing/#substituting-a-custom-user-model

Even though this project isn't "new", it is new in a way since we are still in a prototype stage.

So, let's leave the custom User model definition to save some potential hassle (with migrations) if the need arises to customize the user model. This is one exception to the "rule" against premature optimization.

[brylie]

I am very skeptical to use User = get_user_model() outside the classes and the functions.

Just so long as we adhere to the general Django documentation guidelines, except in the case where referring to User class in the accounts/models.py:

Instead of referring to User directly, you should reference the user model using django.contrib.auth.get_user_model(). This method will return the currently active user model – the custom user model if one is specified, or User otherwise.

https://docs.djangoproject.com/en/3.2/topics/auth/customizing/#referencing-the-user-model

[gorkemarslan]

One additional goal is to deprecate the api app altogether. We will likely need to introduce a new discussions app that contains all code related to Civis, and Threads. Rather than having a dedicated api app, we will have an api.py within the discussions, accounts, and other apps that contain API-related code. The remainder of the code should move to conventional models.py, templates, etc.

@brylie, I aim to get started writing unit tests for accounts/api.py. It will remain unchanged. Right? (Except for the case that some parts from other apps can be moved to the accounts/api.py).

[brylie]

Yes, the accounts app seems to be in a good state so won't change much going forward.

[gorkemarslan]

@brylie, I want to move ProfileSerializer to accounts app and write related unittests as suggested in Task 2 in this issue. I hope it is fine to you.

[brylie]

@gorkemarslan, yes. Let's discuss how to divide up task #1026, as it includes moving the ProfileSerializer among other items.

[gorkemarslan]

@brylie, I am planning to send one more PR for this issue. (Update password activation, profile activation etc views and URLs. Add unit testing.) Then you can close this issue. The new issues that have just be created are more comprehensive than this one.

Otherwise, we will here have circular dependency problem with the same-concept issues. 😛

[brylie]

Please use a GitHub issue keyword to link and close this issue from your pull request.

[gorkemarslan]

I will use. If not, it will be shame on me. You tell me this a couple of times. I will use keywords from now on. :)