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

Feature/order invalidation cronjob #266

Merged
merged 4 commits into from
Mar 11, 2025

Conversation

Jipperism
Copy link
Contributor

Adds a cronjob that validates orders.

  • Validates all valid orders
  • Validates all invalid orders with specific error codes (eg. orders that haven't started yet)

this errors was not caught due to ts-expect-error directive
@Jipperism Jipperism self-assigned this Feb 18, 2025
@Jipperism Jipperism linked an issue Feb 18, 2025 that may be closed by this pull request
Copy link

github-actions bot commented Feb 18, 2025

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 25.09% (🎯 24%) 1094 / 4360
🟢 Statements 25.09% (🎯 24%) 1094 / 4360
🟢 Functions 62.1% (🎯 59%) 59 / 95
🟢 Branches 72.87% (🎯 72%) 180 / 247
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/index.ts 0% 0% 0% 0% 1-58
Generated in workflow #72 for commit 0dc1811 by the Vitest Coverage Report Action

Introduces a new cronjob to periodically validate and potentially invalidate marketplace orders. The cronjob checks all valid orders, and certain invalid orders with specific error codes.
@Jipperism
Copy link
Contributor Author

@bitbeckers @pheuberger tests are failing because treshold is not met. Don't think there's a lot of code worth testing here but also not sure if lowering the treshold is the way to go.

@pheuberger
Copy link
Member

pheuberger commented Feb 21, 2025

@bitbeckers @pheuberger tests are failing because treshold is not met. Don't think there's a lot of code worth testing here but also not sure if lowering the treshold is the way to go.

Honestly I don't have a good idea how to handle these situations. Perhaps we can figure out a good testing strategy for things like this? I agree, lowering the thresholds all the time and raising them back up just creates a tremendous amount of noise.

Alternatively if we think this cannot be reasonably tested, we could ignore these files on coverage tests.

@bitbeckers
Copy link
Collaborator

Alternatively if we think this cannot be reasonably tested, we could ignore these files on coverage tests.

this.

And the refactored API has better testability so we should view these solutions as temporary

@Jipperism
Copy link
Contributor Author

Jipperism commented Feb 24, 2025

Check, added the cron jobs to the ignore list for coverage calculation 👍

it no longer makes sense to keep the error codes in the marketplace sdk
as the only place they will be used is here.
@Jipperism
Copy link
Contributor Author

@bitbeckers I think it would be best to wait for the api refactor to be merged, to add the tests for this as we discussed. That way I can follow the patterns and you mentioned it will have improved testability. Do you agree? And if so, do we want to postpone merging this until the tests are added, or create a new issue for adding tests and merge it already?

@Jipperism Jipperism merged commit 3ffbb0d into develop Mar 11, 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
Status: Done
Development

Successfully merging this pull request may close these issues.

Cron job for order invalidation
3 participants