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

api: Create deletedAt field on assets #1686

Merged
merged 12 commits into from
Apr 14, 2023
Merged

Conversation

victorges
Copy link
Member

@victorges victorges commented Apr 10, 2023

What does this pull request do? Explain your changes. (required)

This is to create a new deletedAt field on assets for us to use in our own billing and monitoring.

It will be updated once the asset is deleted, and we will also send the corresponding webhooks
from that operation.

Specific updates (required)

  • Delete assets via a SQL update instead of the existing markDeleted helper
  • Create deletedAt field and updated it on deletion as well
  • Clean-up VOD webhook payloads by cleaning up task/asset objects as well

How did you test each of these updates (required)
yarn test

  • put on staging and check deletedAt field gets created

Does this pull request close any open issues?
Implements DAT-80

Checklist

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@vercel
Copy link

vercel bot commented Apr 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
livepeer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 13, 2023 11:03pm

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #1686 (c5141a9) into master (73c1082) will increase coverage by 0.08998%.
The diff coverage is 72.34043%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1686         +/-   ##
===================================================
+ Coverage   52.93878%   53.02876%   +0.08998%     
===================================================
  Files             74          74                 
  Lines           4900        4903          +3     
  Branches         972         976          +4     
===================================================
+ Hits            2594        2600          +6     
+ Misses          1972        1970          -2     
+ Partials         334         333          -1     
Impacted Files Coverage Δ
...rc/controllers/experiment/lit-signing-condition.ts 4.16667% <0.00000%> (ø)
packages/api/src/middleware/hardcoded-nodes.ts 72.72727% <ø> (-10.60606%) ⬇️
packages/api/src/app-router.ts 51.28205% <20.00000%> (ø)
packages/api/src/parse-cli.ts 34.04255% <50.00000%> (+2.33523%) ⬆️
packages/api/src/task/scheduler.ts 49.34211% <62.50000%> (+0.67544%) ⬆️
packages/api/src/controllers/task.ts 35.55556% <90.00000%> (+3.97661%) ⬆️
packages/api/src/controllers/asset.ts 59.06593% <93.75000%> (+0.54945%) ⬆️
packages/api/src/test-server.ts 93.02326% <100.00000%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73c1082...c5141a9. Read the comment docs.

Impacted Files Coverage Δ
...rc/controllers/experiment/lit-signing-condition.ts 4.16667% <0.00000%> (ø)
packages/api/src/middleware/hardcoded-nodes.ts 72.72727% <ø> (-10.60606%) ⬇️
packages/api/src/app-router.ts 51.28205% <20.00000%> (ø)
packages/api/src/parse-cli.ts 34.04255% <50.00000%> (+2.33523%) ⬆️
packages/api/src/task/scheduler.ts 49.34211% <62.50000%> (+0.67544%) ⬆️
packages/api/src/controllers/task.ts 35.55556% <90.00000%> (+3.97661%) ⬆️
packages/api/src/controllers/asset.ts 59.06593% <93.75000%> (+0.54945%) ⬆️
packages/api/src/test-server.ts 93.02326% <100.00000%> (ø)

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

LGTM. Added one super nit comment.

One more general comment, I think this PR both adds new functionality (deletedAt), but also does some refactoring. I think it'd be better to split it into a separate refactoring PR in the future. Simpler to review (and rollback if needed).

packages/api/src/controllers/asset.ts Show resolved Hide resolved
@victorges victorges merged commit ad8d909 into master Apr 14, 2023
18 checks passed
@victorges victorges deleted the vg/feat/updated-at-deletion branch April 14, 2023 18:49
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.

None yet

2 participants