slashmili / django-jalali

Jalali DateField support for Django model
http://pypi.python.org/pypi/django_jalali
BSD 3-Clause "New" or "Revised" License
258 stars 52 forks source link

tzinfo removed from jDateTimeField value for mysql #141

Closed MAM-SYS closed 3 years ago

MAM-SYS commented 3 years ago

Hello Friends This issue #86 happens when USE_TZ = True and our DATABASE is MySQL I found out that we can solve this problem by removing tzinfo from value of the jDateTimeField only for MySQL So if this is not the best approach and need to be change, just let me know I'll make it better if needed With Respect

hramezani commented 3 years ago

Hello @MAM-SYS, Thanks for your effort. Have you tested your patch with MySQL database? Could you please add a test to confirm the fix?

MAM-SYS commented 3 years ago

Hello Friend @hramezani Sorry it takes too long Somethings went wrong with my last patch I fixed them and made a mechanism to run all old tests based on mysql database.and it seems all passed :) I wish it can help but if still anything needs to be changed, just tell me Merci

hramezani commented 3 years ago

@MAM-SYS Thanks for your update. First, I think we need to add another test job to Github action to run tests against MySQL.(in a separate PR or separate commit) Then we can have an automated test on MySQL.

BTW, I can check your PR later(unfortunately I don't have time now to check it)

MAM-SYS commented 3 years ago

@hramezani Thanks for your response and help Sorry if i missed something again I wait for your call if anything needs to be changed for this PR

MAM-SYS commented 3 years ago

Hello Dear @hramezani I worked some more on this issue to make it better Here is the summary of what i did : 1 - I add my patch for mysql support 2 - Since i wanted to run tests based on two different databases so i separate setting to this : settings/ ├── init.py ├── base.py ├── test1.py ├── test2.py 3 - I added a test job to Github action to run tests against MySQL

So at first glance, if you think these changes seem ok, I can send the changes Thanks

hramezani commented 3 years ago

@MAM-SYS Thanks for your update. Sure! Please push your changes. Unfortunately I am busy now 😕 I will check it probably next week

hramezani commented 3 years ago

@MAM-SYS I've run our main branch tests on my local machine against a MySQL database and it was successful. I think we need to add some regression tests to prove your patch will fix the problem. Without a test, we don't know which problem are we going to fix. Thanks!

MAM-SYS commented 3 years ago

Hello Dear @hramezani I don't know how to say sorry about this It was my mistake This problem seems to be on older versions of MySQL database and also MariaDB new version Screenshot from 2021-07-19 20-25-49 Unfortunately first time i tested this issue on MariaDB and when i faced with this issue, i thought this problem still exists on MySQL So this patch is more accurate for MariaDB (but that was not our goal) So I do not want to bother you anymore for this issue with my mistakes If one day this package needs a patch for MariaDB, I think i can help then Otherwise, apologize me

hramezani commented 3 years ago

@MAM-SYS Thanks for letting me know and thanks for your effort. Yeah, we can use your fix whenever somebody reported this problem on MariaDB.

I close this PR for now.