noexpects / django_test_task

0 stars 0 forks source link

Review results #1

Open anndoc opened 3 years ago

anndoc commented 3 years ago
  1. Add .gitignore file. The .idea, venv, __pycache__, *.pyc should not be committed.
  2. PEP8: do not use camel case for app names
  3. Add requirements.txt (Two Scoops of Django book chapter 5.5 http://bedford-computing.co.uk/learning/wp-content/uploads/2016/08/Daniel-Roy-Greenfeld-Audrey-Roy-Greenfeld-Two-Scoops-of-Django_-Best-Practices-for-Django-1.8-Two-Scoops-Press-2015.pdf )
  4. Never import user model directly, use get_user_model() instead
  5. All api keys or settings which can be changed often (for example settings.DEBUG, settings.ALLOWED_HOSTS) should be stored in the .env file. This is bad practice
    SECRET_KEY = 'django-insecure-(3yqc6jeq3$lxc*%0$d*#og33y_z9i)m15i*+v70533cq7oq)j'

    use django-environ instead https://django-environ.readthedocs.io/en/latest/

  6. https://github.com/noexpects/django_test_task/blob/f0174e189d6576f062e815c945236631a2647777/MyApp/templatetags/user_filters.py#L11 13 is a magic number, it's better to create a variable, for exampleUSER_ALLOWED_AGE, in the settings or in the constants.py file for that.
  7. class DataMixin(object): just class DataMixin: object is redundant in the python3
  8. That's cool that you use mixins but I would avoid to create mixins just for model / success_url . In big projects that cause problems, usually mixins are used for duplicated methods. And it's also better to store them in the separate mixins.py files.
  9. Using class-based views together with functional views is the nightmare for people who will work with code in the future. Can you please replace it with class-based view?
  10. It's a good practice to add paginate_by to the list view. Just to avoid retrieving thousand users.

Tests:

  1. Instead of
        user = User.objects.create(username='Alex',
                                   birth_date='2002-10-03',
                                   password='ewqtre231')

    use factory-boy https://factoryboy.readthedocs.io/en/stable/ It will be something like:

    
    ./tests/factories.py
    import random

import factory from django.contrib.auth import get_user_model TEST_USER_PASSWORD = 'P@ssw0rdP@ssw0rd' User = get_user_model()

class UserFactory(factory.django.DjangoModelFactory): email = factory.Sequence(lambda n: f'user-{n}-{random.randint(1, 1000)}@example.com') password = factory.PostGenerationMethodCall('set_password', TEST_USER_PASSWORD) first_name = factory.Faker('first_name') last_name = factory.Faker('last_name')

class Meta:
    model = User
2. It's usually better to move repeated code in the tests to the setUp
def setUp(self):
    self.user = UserFactory()
    self.url = reverse('user-list')
    self.detail_url = reverse('user-detail', kwargs={'pk': self.user.pk})
3. It's also better to check that there is expected data in the response, for example 
def test_list_get_200(self):
    ....................
    self.assertContains(response, 'User List')
    **# also it's better to add some users to this tests, or even better create anew one with users, otherwise it's not guarantee that the view does not raise an exception when there will be data to retrieve**

def test_create_post(self):    
    ...........................
    user = User.objects.last()
    self.assertEqual(user.username, 'Alex')
    self.assertEqual(user.birth_date, datetime.date(1995, 12, 12))
    self.assertTrue(user.has_usable_password())

4. No tests for template tags. It's better to create separate files test_views.py, test_template_tags.py, test_models.py etc.        
anndoc commented 3 years ago

1. Как из функции которая выдает csv файл сделать класс. Со стандартными джанговских классами все более-менее понятно, а если свой пишешь? Все что успел нагуглить до того как ебнул инет было про отрисовку списков или еще чего-нибудь, все наследовалось от ListView, CreateView и т.д.

In case if you need to write some custom view you can inherit from django.views.View and implement there get / post methods, for example

from django.views import View

class UserCSVImportView(View):
    def get(self, request, *args, **kwargs):
        response = HttpResponse(
            content_type='text/csv',
            headers={'Content-Disposition': 'attachment; filename="Users.csv"'},
        )
        writer = csv.writer(response)
        writer.writerow(['Username', 'Birth date', 'Age validation', 'Random number', 'BizzFuzz'])
        for user in User.objects.all():
            writer.writerow([user.username, ])
        return response

Since you need to export list of users you can inherit from django.views.generic.list.BaseListView. It's already has get_queryset method so you don't need to implement own

from django.views.generic.list import BaseListView

class UserCSVImportView(BaseListView):
    model = User
    filename = 'Users.csv'
    fields = ('Username', 'Birth date', 'Age validation', 'Random number', 'BizzFuzz')

    def get(self, request, *args, **kwargs):
        response = HttpResponse(
            content_type='text/csv',
            headers={'Content-Disposition': f'attachment; filename="{self.filename}"'},
        )
        writer = csv.writer(response)
        writer.writerow(self.fields)
        for user in self.get_queryset():
            writer.writerow([user.username])
        return response

2. Второй момент это как на стандартные эти классы натянуть стили? Если посмотришь в создании пользователя поля стандартные. Из-за того что там form.as_p получилось сделать полукостыль в виде стиля тега <p> в style css

You can get access to the fields in the template in a loop {% for field in form %}. In this case you will able to customize general form but not add styles to the each form input. If that's not enough there are bunch of django apps which allow you to add css class / attributes to the form fields. I used to use the django-widget-tweaks https://github.com/jazzband/django-widget-tweaks , not sure if it's still popular. If you were able to get the latest version of Two Scoops of Django books you can check there what they recommend.

{% extends 'base.html' %}

{% load i18n %}
{% load widget_tweaks %}

{% block content %}
<form class="needs-validation{% if form.errors %} was-validated{% endif %}" novalidate
      method="post"
      action="{% if object %}{% url 'users:update' object.pk %}{% else %}{% url 'users:create' %}{% endif %}">
    {% for field in form %}
      <div class="col-md-4 mb-3">
        <label class="control-label" for="{{ field.id_for_label }}">{{ field.label }}</label>
        {% render_field field placeholder=field.label class='form-control' %}
        {% if field.errors %}
          <div class="invalid-feedback">
            {% for error in field.errors %}
              {{ error }}<br/>
            {% endfor %}
          </div>
        {% endif %}
      </div>
    {% endfor %}
    <div class="col-md-4 mb-3">
      <button class="btn btn-primary" type="submit">{% trans 'Save' %}</button>
    </div>
</form>
{% endblock content %}