Closed laricko closed 1 year ago
Hi @chvmq, thanks for the PR. I will take a closer look as soon as I have some time, possibly this weekend. Could you in the meantime make sure that the tests and linting rules still pass? Running a quick test against your branch makes it break for me.
@chvmq sorry, the flake8 pre-commit action was not running properly as they moved the repo. I've updated it, you can do the checks now.
Merging #23 (215b7b4) into master (bae3762) will increase coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #23 +/- ##
=======================================
Coverage 99.94% 99.94%
=======================================
Files 50 50
Lines 1962 1978 +16
=======================================
+ Hits 1961 1977 +16
Misses 1 1
Impacted Files | Coverage Δ | |
---|---|---|
salesman/basket/views.py | 100.00% <100.00%> (ø) |
|
salesman/checkout/views.py | 100.00% <100.00%> (ø) |
|
salesman/conf.py | 100.00% <100.00%> (ø) |
|
salesman/orders/views.py | 100.00% <100.00%> (ø) |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
@dinoperovic how to start test? I have pytest 7.2.1 when I start pytest I got this error
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov=. --cov-report=html
inifile: /home/lari/projects/django-salesman/pytest.ini
rootdir: /home/lari/projects/django-salesman
seems like my pytest can't parse this line pytest: error: unrecognized arguments: --cov=. --cov-report=htm
when I comment that line my test running but I got another issue
django.core.exceptions.ImproperlyConfigured: Requested setting INSTALLED_APPS, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.
UPD: so it turned out that I need pytest-cov. but I still have second error
UPD2: again I just needed to install pytest-django :)
I thought that I already download it in my venv. which command could install all dev depends?
@laricko to run the tests you should do:
poetry install -E tests
poetry run pytest
Also check for lint errors:
poetry run pre-commit run --all-files
@dinoperovic I think I am done with my pull request. How do you feel about that feature?
Hey @laricko, thanks for the PR, I will take a closer look this weekend.
Hey @laricko, I went over your changes and I like the feature. I did however do some updates:
moved the validation from serializer to the view so it applies to GET /checkout/
as well
used 403 forbidden error code instead of 400
updated your test to include GET version and added some comments
updated view action mapping method naming to be consistent with the original action name.
I realise methods pay
and pay_create
are a bit strangely named, but I will look to update the endpoint entirely to payment
and payment_create
in the future.
added back the dict
evaluation in get_basket_response
.
I remember adding this because of some strange edge-case where the serializer.data
is returned as proxy ReturnDict
and not a dictionary. Can't exactly remember when it happens, possibly when using ?basket
on other endpoints, but would like to avoid this possible regression.
updated docs a bit, added contributors to credits :)
Thank you for contributing to Salesman, before you continue make sure that:
poetry run pytest
poetry run pre-commit run --all-files