Skip to content
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

Merged

Conversation

hamed-valiollahi
Copy link
Collaborator

This PR adds a 'Compliance period' column to the Transaction table for both IDIR and BCeID users.

Closes #1719

Copy link

github-actions bot commented Jan 25, 2025

Frontend Test Results

  1 files  ±0  125 suites  ±0   44s ⏱️ -3s
436 tests ±0  416 ✅ ±0  20 💤 ±0  0 ❌ ±0 
438 runs  ±0  418 ✅ ±0  20 💤 ±0  0 ❌ ±0 

Results for commit 4510a5a. ± Comparison against base commit e035a9d.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 25, 2025

Backend Test Results

524 tests  ±0   524 ✅ ±0   1m 49s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 4510a5a. ± Comparison against base commit e035a9d.

♻️ This comment has been updated with latest results.

)
description = Column(
String,
primary_key=True,
Copy link
Collaborator

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?

Copy link
Collaborator Author

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
            );
        """
    )

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@AlexZorkin
Copy link
Collaborator

This looks good, just repoint it to out latest release branch release-1.0.0

@hamed-valiollahi hamed-valiollahi changed the base branch from release-0.2.0 to release-1.0.0 January 31, 2025 01:59
@hamed-valiollahi hamed-valiollahi changed the base branch from release-1.0.0 to release-0.2.0 January 31, 2025 02:00

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@hamed-valiollahi hamed-valiollahi force-pushed the feat/hamed-add-compliance-period-1719 branch from 0fe42c5 to 87aec8d Compare January 31, 2025 02:10
@hamed-valiollahi hamed-valiollahi changed the base branch from release-0.2.0 to release-1.0.0 January 31, 2025 02:10
@hamed-valiollahi
Copy link
Collaborator Author

This looks good, just repoint it to out latest release branch release-1.0.0

Sure, I've repointed it to our latest release branch, release-1.0.0.

Copy link
Collaborator

@AlexZorkin AlexZorkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hamed-valiollahi hamed-valiollahi merged commit 59468a6 into release-1.0.0 Feb 3, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LCFS - add Compliance period column to the Transaction table
2 participants