-
Notifications
You must be signed in to change notification settings - Fork 50
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
WIP:Refactor lại models thành module và chuyển sang dùng Enum field cho các status choices #248
WIP:Refactor lại models thành module và chuyển sang dùng Enum field cho các status choices #248
Conversation
django 3 introduce enum fields for choices that is easier to maintain, also, remove the hardcoded ordinal number for the models
Adding new fields `do_quan_trong`, `trang_thai_lien_lac` and `nhu_cau`
Hình như bạn chưa join Slack à? https://join.slack.com/t/cuuhomientrung/shared_invite/zt-irc2ky4w-xfWiwtwLQWL4BlqYSfFgfg Mấy hôm nay mình chỉ kiểm tra bên Issues mà không kiểm tra Pull Requests nên không để ý thấy cái PR này. |
from app.models import TinTuc, TinhNguyenVien, CuuHo, HoDan, Tinh, Huyen, Xa,\ | ||
TrangThaiHoDan, CUUHO_STATUS, TINHNGUYEN_STATUS | ||
TrangThaiHoDan, RescueStatus, VolunteerStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Đề xuất: CuuHoStatus
, TinhNguyenVienStatus
.
Nếu đã đổi tên thì phải đổi tên hết (e.g. CuuHo
-> Rescue
). Vấn đề là đổi tên như vậy mọi người đọc không hiểu.
model_name='historicalhodan', | ||
name='trang_thai_lien_lac', | ||
field=models.IntegerField(choices=[(0, 'Không rõ'), (1, 'Không liên lạc được'), (2, 'Liên lạc được')], default=0, help_text='Tình trạng liên lạc của hộ dân', verbose_name='Trạng Thái Liên Lạc'), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Model historicalhodan
có vẻ không còn được dùng nữa. Mình nghĩ không nên AddField
vào cái model này.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mình cũng không biết là có còn dùng hay ko nên mình ko thay đổi gì cái relation đó. Bạn có thể confirm lại giúp mình xem cái relation này nếu ko dùng nữa thì mình có thể bỏ đi và tạo lại migration và sửa những chỗ bạn comment luôn 1 thể. Trong thời gian chờ confirm mình sẽ để PR này thành WIP nhé.
class HoDan(models.Model):
...
history = HistoricalRecords(
history_change_reason_field=models.TextField(null=True),
bases=[
IPAddressHistoricalModel,
],
)
{% endfor %} | ||
{% else %} | ||
<p>{% translate 'You don’t have permission to view or edit anything.' %}</p> | ||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mình nghĩ việc đánh số chỉ là để quy định thứ tự thôi. Nếu bỏ đánh số đi mà mọi thứ vẫn được sắp xếp theo thứ tự cũ thì bỏ đánh số đi cũng được. Việc customize app_list.html
chưa đem lại nhiều lợi ích và đang làm tăng độ phức tạp của code base, đi ngược lại với định hướng của giai đoạn hiện tại.
Tham khảo: Định hướng của Gen 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok nếu số thứ tự ko phải là yêu cầu thì mình bỏ đi cũng đc. Thực ra app_list.html
này là copy của django admin ra thôi, nên gần như không khác j cái mặc định, chỉ có thêm phần số thứ tự thôi. Có nhiều cái mình không rõ là do yêu cầu cần có hay chỉ đơn giản là nice to have nên mình ko dám thay đổi hết, mà chỉ thay đổi phương pháp 😓 .
|
||
class Meta: | ||
verbose_name_plural = "6. Tin tức quan trọng " | ||
verbose_name = "Tin tức quan trọng " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chỗ refactor này dài quá, mình không review được. Vậy nên mình giả sử rằng bạn chỉ refactor và không làm thay đổi logic nha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phần này mình refactor với mục đích để quản lý code và tăng readability chứ ko thay đổi gì về logic cả. Như mình nói trong phần description, đó là ko ảnh hưởng gì tới import ở các modules khác
Tất cả các models dồn hết vào một file
models.py
làm khó đọc và không maintainable, nên mình chuyểnmodels
thành 1 module và các file nhỏ. Không ảnh hưởng tới cácimport
hiện đang dùng.Bỏ hardcode số thứ tự ở trong tên của các model đi, thay bằng template
{{ forloop.counter }}
trongadmin/app_list.html
cho dynamicDjango 3 có những enum fields để thay thế cho các
list/tuple
choices nên mình chuyển qua dùng. Cũng để tăng readability của code nữa. Cái này không ảnh hưởng gì tới database và không cần migration.Trong Chỉnh sửa model "HoDan". Thêm trang "Thêm hộ dân", "Chỉnh sửa hộ dân", "Xem hộ dân". #208 mình thấy có đề xuất thêm field cho model
HoDan
nên mình nhân tiện làm luôn, vì bạn @DangHoangGeo nhận làm rồi nên 2 cái commit cuối về model này nếu bạn cảm thấy dùng được thì ta có thể dùng luôn, còn nếu bạn thấy không được thì ta có thể bỏ.