-
Notifications
You must be signed in to change notification settings - Fork 34
Add created_at and created_by columns to series_sales table. #483
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
Conversation
1b23161
to
68cdc99
Compare
<constraints nullable="false" /> | ||
</column> | ||
<column name="created_by" type="INTEGER" | ||
defaultValueComputed="SELECT u.id FROM users u WHERE u.role = 'ADMIN'"> |
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, we're fetching all admins instead of the first one.
<comment>Adds created_at and created_by columns to series_sales table and fills them with default data</comment> | ||
|
||
<addColumn tableName="series_sales"> | ||
<column name="created_at" type="DATETIME" defaultValueComputed="${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.
To make it compatible with MySQL < 5.6.6 let's do it in the 3 steps then: 1) create nullable field 2) execute update query 3) mark field as not null
Also put comment why we're doing this in a such way.
http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.0.xsd"> | ||
|
||
<changeSet id="add-creator-columns-to-series_sales_table" author="cssru" context="scheme"> | ||
<comment>Adds created_at and created_by columns to series_sales table and fills them with default data</comment> |
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.
s/with default data/with default values/ or even remove this comment (because it obvious).
defaultValueComputed="SELECT u.id FROM users u WHERE u.role = 'ADMIN'"> | ||
<constraints nullable="false" | ||
references="users(id)" | ||
foreignKeyName="fk_series_sales_created_by" /> |
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 should have name fk_series_sales_user_id
.
68cdc99
to
743a303
Compare
15130c5
to
0052ddb
Compare
Changes Unknown when pulling 0052ddb on cssru:gh469_add_created_to_sales into * on php-coder:master*. |
http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.0.xsd"> | ||
|
||
<changeSet id="add-creator-columns-to-series_sales_table" author="cssru" context="scheme"> | ||
<comment>We use this way because of MySQL version supported by TravisCI (lower than 5.6.6)</comment> |
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.
This should be a XML comment <!-- and -->
instead of comment on changeSet.
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.
And let's update a comment because it's not about TravisCi's version but about my attempt to support a bit more versions of MySQL:
Since MySQL 5.6.5 it's possible to use `NOW()` as default value for a column but we're doing the same in 3 steps to support also old versions.
BTW, did you know that NOW()
returns the same value during transaction? I didn't.
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.
No, I didn't.
<update tableName="series_sales"> | ||
<column name="created_by" | ||
type="INTEGER" | ||
valueComputed="(SELECT u.id FROM users u WHERE u.role = 'ADMIN' LIMIT 1)" /> |
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.
ORDER by u.id
is missing here.
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.
Done.
…them with default values.
0052ddb
to
1fe081e
Compare
Merged in d55cd6c |
Add created_at and created_by columns to series_sales table and fill them with default values.
Addressed to #469