-
Notifications
You must be signed in to change notification settings - Fork 26
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
Bugfix/localization 926 #478
Conversation
f48100f
to
92204b4
Compare
'PASSWORD': '', | ||
'HOST': '', | ||
'PORT': '', | ||
'ATOMIC_REQUESTS': True, | ||
'CHARSET':'utf8mb4', |
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.
Do we need to override CHARSET and COLLATION now that they are in 90-local.conf.template
file? (90-local.conf file will be loaded before 91-travis.conf)
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.
But is Travis loading 90-local.conf.template
on the build environment? It seems to me that It should not run it since it has the .template
extension
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.
Any input on this Igor? As I stated above It's not clear to me that travis will use the configuration in 90-local.conf.template
'NAME': '', | ||
'CHARSET': 'utf8', | ||
'NAME': 'test_zing', | ||
'CHARSET':'utf8mb4', |
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.
Same question as above.
if DATABASE_BACKEND.startswith("mysql"): | ||
DATABASES['default']['ENGINE'] = 'django.db.backends.mysql' | ||
DATABASES['default']['NAME'] = 'zing' | ||
if DATABASE_BACKEND.startswith("sqlite"): |
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.
Can you please explain this change? I think with this PR we're moving away from sqlite.
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.
It's to maintain sqlite support for user's that are on it. Thought I should keep it for compatibility
"mtime": 1578661829, | ||
"source": "Translated Source /language0/project0/subdir0/sto\u2026", | ||
"mtime": 1591122676, | ||
"source": "Translated Source /language0/project0/store0.po 2\u2026", |
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.
Here and below, I'm concerned about these snapshot changes to the source string. They don't seem to be related to the changes this PR is supposed to introduce. We need to understand why this property is reported differently now.
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.
Don't know why it happens. Tests wouldn't pass if I kept the old snapshots. Might be because of my environment, but before the changes I introduced the tests would run smoothly.
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.
The thing is that when I run the tests without generating a new dump (not snapshots) I get this error on all tests when setting up the fixtures.
Then, when I generate a new dump I can get past the fixtures setup but other errors show up.
When I called the submission procedure outside the save method, generating a new dump and snapshots would fix all tests. However, there's that weird modification of source strings.
So the problem here would be that the original data_dump.json
is having troubles when initialized on MySQL. Apparently because of uniqueness constraints. On the other hand when I create a new dump some of this problems dissapear.
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.
@iafan It turns out I could not get over the uniqueness constraints colliding from the dump without generating a new dump, so I uploaded a new one. It contains some changes on ids, timestamps and ordering.
The new dump would not pass all tests without regenerating snapshots, so I did regenerate them. The new snapshots have some weird source changes as you pointed out. I've looked back at commit history and found a commit that had a similar impact on snapshots. The last modification of urls to be tested occurs here, so I discard the source modifications are due to different urls being tested and just having out of date snapshots.
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.
Hello @mandrade2 I have a couple of questions:
Just for future reference, why are there some many files being updated?
Have you updated the SQL to look better? (Formatted)
Also please mark as resolved all comments made by other reviewers if they were addressed.
I fixed the remaining issues regarding tests. It turns out that I had to modify the submission save method to call for a reload from the database, to avoid Django raising an error because of garbage collecting an unsaved instance. Also added to the stored procedure arguments for a suggestion_id and quality_check_id, I had forgotten them. I rolled back all the snapshot changes and fixed two tests that kept failing. The |
55a5f7a
to
34840fd
Compare
a4561d3
to
7c7a204
Compare
7c7a204
to
76506ef
Compare
76506ef
to
4d281a9
Compare
In this PR issue #253 is resolved. Whenever a submission is being saved, a stored procedure checks that with a
SELECT ... FOR UPDATE
row lock that the submission to be saved is not there yet. This prevents duplicate submission being saved. Making rows unique setting a multi-column index or unique_together in Django would've been preferred, but the older duplicate submissions needed to be there for consistency. Besides, Django does not support QuerySet.select_for_update() For MySQL databases.Travis jobs where configured to run with Mysql, so the migration and testing job succeed.
All database defaults where set to have a correct character set and collation. Since by default MySQL does not store 4 byte-characters for the utf8 character set. It's explained here https://stackoverflow.com/a/20349552/8763522