Skip to content
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

Introduction of cart workflow #63

Merged
merged 58 commits into from
Nov 16, 2024
Merged

Introduction of cart workflow #63

merged 58 commits into from
Nov 16, 2024

Conversation

6R1M5L07H
Copy link
Contributor

@6R1M5L07H 6R1M5L07H commented Oct 28, 2024

The feature includes two models: cart and cart item. The cart is created as an intermediate step between the selection of the goods so that a customer can collect items in said cart as cart items and check them out in one step.

Resolves request #69

Copy link
Owner

@ilyarolf ilyarolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still a lot of work to be done before pouring in that PR.
General Comments:

  1. Use an approach that is already in use at the moment:
  • Commas at the beginning instead of at the end;
  • Distinguish models for SQLAlchemy from previous models;
  • Products in cart can be shown the same way as Subcategories.
    All this is unnecessary and makes the code heterogeneous.
  1. There should not be any comments in the files, only TODO is allowed.
  2. Do not add useless files that are not used anywhere, even if you plan to use the code from these files later, they are of no use now.
  3. Don't create formatting changes, this causes the file to show up as changed, even though the actual essence of the file hasn't changed. This increases the cognitive load for the PR checker.

.env Outdated Show resolved Hide resolved
.env.template Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
handlers/user/all_categories.py Outdated Show resolved Hide resolved
handlers/user/all_categories.py Outdated Show resolved Hide resolved
services/cart.py Outdated Show resolved Hide resolved
services/cart.py Show resolved Hide resolved
services/buyItem.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
(cherry picked from commit 9da2cfa)
@6R1M5L07H
Copy link
Contributor Author

6R1M5L07H commented Nov 5, 2024

* Commas at the beginning instead of at the end;

Done

* Distinguish models for SQLAlchemy from previous models;

Please elaborate

* Products in cart can be shown the same way as Subcategories.
  All this is unnecessary and makes the code heterogeneous.

IMO these are two separate entities. If any, the CartItem resembles more the Buy Entity but still I think that you need a different entity. Here is why:

  • Subcategory (which I would rephrase as "Article" tbh, but that is something different) is something static in the DB and is independent from a User's class or a Buy, so it exists from my understanding to represent the Article Catalogue
  • Buy is bound to the User and exists for the purpose of having a history
  • CartItem however is meant to be volatile since it only exists while the Cart exists and the Cart only exists until its content - the CartItems - are transformed into Buy objects. The CartItem is imo needed to implement the payment process on a "pay as you go" approach instead of the existing deposit approach

Happy to discuss

2. There should not be any comments in the files, only TODO is allowed.

I'll follow your direction, although I think that comments don't hurt anyone. I had to understand your way of thinking for some time and comments would have helped me a lot. If you change your point of view here, let me know, please. Until then, I remove the comments (I do not mean commented code but actual comments btw)

3. Do not add useless files that are not used anywhere, even if you plan to use the code from these files later, they are of no use now.

Agreed and removed.

4. Don't create formatting changes, this causes the file to show up as changed, even though the actual essence of the file hasn't changed. This increases the cognitive load for the PR checker.

Agreed, will improve myself here, thank you

@6R1M5L07H 6R1M5L07H changed the title Introduction of cart workflow (not yet production ready) Introduction of cart workflow Nov 6, 2024
@6R1M5L07H 6R1M5L07H changed the base branch from master to develop November 8, 2024 01:06
# Conflicts:
#	handlers/user/all_categories.py
#	l10n/de.json
#	l10n/en.json
#	run.py
#	services/category.py
#	utils/notification_manager.py
# Conflicts:
#	handlers/user/all_categories.py
#	l10n/de.json
#	l10n/en.json
#	run.py
#	services/category.py
#	utils/notification_manager.py
@ilyarolf
Copy link
Owner

@6R1M5L07H

  1. About SQLAlchemy models.
    image
    image
    Use a model field format similar to Users, i.e.
    Column({type}, nullable={value}, etc...)

Please elaborate

  • Products in cart can be shown the same way as Subcategories.
    All this is unnecessary and makes the code heterogeneous.

What was meant here is that you need to rework the way you describe the model fields. What I described in the 1st paragraph with screenshots.

  1. About the comments in the code. Try to use inline comments only in complex cases, for example in the db.py file it is described in detail why it is necessary to import SQLAlchemy models even though they are not used in this file, and also it is described why SQLCipher will not work on Windows.
    Comments that I will always be glad to welcome are PEP8 docstrings.

ilyarolf and others added 4 commits November 15, 2024 18:10
Added pagination to the cart.
The functionality related to the “CartItem” entity has been moved to a separate service.
Cart is initialized for each user during registration.
Overall code refactoring was done.
removed .env.template, removed redundant constants from .env
The “Cart” and “CartItem” table models have been reworked.
handlers/user/all_categories.py Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
handlers/user/all_categories.py Outdated Show resolved Hide resolved
@ilyarolf ilyarolf merged commit dde160c into ilyarolf:develop Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants