-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feat: Add compliance period column to transaction table - 1719 #1799
Feat: Add compliance period column to transaction table - 1719 #1799
Conversation
) | ||
description = Column( | ||
String, | ||
primary_key=True, |
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.
I don't think this needs to be a primary_key?
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.
I noticed that the unique index in the migration is a combination of transaction_id
, description
, and transaction_type
, which is why I included it in the model. Should I remove it from the migration as well?
op.execute(
"""
CREATE UNIQUE INDEX mv_transaction_aggregate_unique_idx
ON mv_transaction_aggregate (
transaction_id,
description,
transaction_type
);
"""
)
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.
Yes I don't think its required to have description as a third primary element. transaction_type and transaction_id will be unique enough.
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.
Thanks, Alex, for reviewing my code! I've removed the description as a primary key in both the migration and model.
This looks good, just repoint it to out latest release branch release-1.0.0 |
0fe42c5
to
87aec8d
Compare
Sure, I've repointed it to our latest release branch, |
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 PR adds a 'Compliance period' column to the Transaction table for both IDIR and BCeID users.
Closes #1719