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.

Sign up for GitHub

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

Jump to bottom

feat: add created_at and updated_at to wallets and accounts #2177

Merged
merged 13 commits into from
Dec 21, 2023
Merged

feat: add created_at and updated_at to wallets and accounts #2177

merged 13 commits into from
Dec 21, 2023

Conversation

dni
Copy link
Member

@dni dni commented Dec 13, 2023

the title says it all :)

motorina0
motorina0 reviewed Dec 13, 2023
View reviewed changes
lnbits/core/crud.py Outdated Show resolved Hide resolved
@dni dni requested a review from motorina0 December 13, 2023 11:46
motorina0
motorina0 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Collaborator

@motorina0 motorina0 left a comment •
edited

Choose a reason for hiding this comment

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

utACK
ps: tests fail

jackstar12
jackstar12 reviewed Dec 13, 2023
View reviewed changes
lnbits/core/crud.py Outdated Show resolved Hide resolved
prusnak
prusnak requested changes Dec 13, 2023
View reviewed changes
Copy link
Collaborator

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

CI failed

@codecov Codecov
Copy link

codecov bot commented Dec 13, 2023 •
edited

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (165d749) 59.29% compared to head (5148906) 52.76%.
Report is 1 commits behind head on dev.

Files Patch % Lines
lnbits/core/crud.py 66.66% 4 Missing ⚠️
lnbits/core/migrations.py 80.00% 3 Missing ⚠️
lnbits/db.py 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##              dev    #2177      +/-   ##
==========================================
- Coverage   59.29%   52.76%   -6.54%     
==========================================
  Files          59       59              
  Lines        8871     8906      +35     
==========================================
- Hits         5260     4699     -561     
- Misses       3611     4207     +596

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@prusnak prusnak mentioned this pull request Dec 14, 2023
Merged
motorina0
motorina0 reviewed Dec 15, 2023
View reviewed changes
lnbits/db.py Outdated Show resolved Hide resolved
prusnak
prusnak requested changes Dec 19, 2023
View reviewed changes
Copy link
Collaborator

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

this shouldn't be merged yet - there are still unaddressed comments about leaking abstraction from db.py - changing to draft state

@prusnak prusnak marked this pull request as draft December 19, 2023 11:18
@dni dni requested a review from prusnak December 20, 2023 07:43
@dni dni marked this pull request as ready for review December 20, 2023 08:30
motorina0
motorina0 approved these changes Dec 20, 2023
View reviewed changes
Copy link
Collaborator

@motorina0 motorina0 left a comment

Choose a reason for hiding this comment

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

tACK

Nitpick:

  • all existing wallets and accounts will have created_at the date of the migration
  • for a backwards valid data set a script can be created to set created_at at the first payment date

dni and others added 3 commits December 20, 2023 11:51
@dni
int(time()) proper default values in migration
e0f68fe
@dni
uncomment migration
741369a
@prusnak
use now = int(time()) idiom to make code more readable
5148906 
also this fixes the issue where time() is called multiple times
providing different return values for multiple invocations
@dni dni requested a review from motorina0 December 21, 2023 09:18
@arcbtc arcbtc merged commit 815c3e6 into dev Dec 21, 2023
22 checks passed
@arcbtc arcbtc deleted the timestamp-wallets branch December 21, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prusnak prusnak prusnak approved these changes

@motorina0 motorina0 motorina0 approved these changes

@talvasconcelos talvasconcelos talvasconcelos approved these changes

@jackstar12 jackstar12 Awaiting requested review from jackstar12

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dni @prusnak @motorina0 @talvasconcelos @jackstar12 @arcbtc