check-api icon indicating copy to clipboard operation
check-api copied to clipboard
verified publisher icon meedan

Epic/cv2 5050 text vectorization via presto

Open DGaffney opened this issue 1 year ago • 1 comments

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context – why has this been changed/fixed.

References: TICKET-ID, TICKET-ID, …

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can verify the changes. Please describe whether or not you implemented automated tests.

Things to pay attention to during code review

Please describe parts of the change that require extra attention during code review, for example:

  • File FFFF, line LL: This refactoring does this and this. Is it consistent with how it’s implemented elsewhere?
  • Etc.

Checklist

  • [ ] I have performed a self-review of my own code
  • [ ] I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • [ ] I have added regression tests, if the PR fixes a bug
  • [ ] I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • [ ] I considered secure coding practices when writing this code. Any security concerns are noted above.
  • [ ] I have commented my code in hard-to-understand areas, if any
  • [ ] I have made needed changes to the README
  • [ ] My changes generate no new warnings
  • [ ] If I added a third party module, I included a rationale for doing so and followed our current guidelines

DGaffney avatar Sep 10 '24 17:09 DGaffney

Is it all ready to be merged with develop, Devin? 🤔

Sorry, no, I was opening this to test something - will set to draft!

DGaffney avatar Sep 11 '24 13:09 DGaffney

@DGaffney , thanks for all the work here! Can you please make sure that we have tests that cover these lines (or confirm if these lines should be removed, or if the fact that these lines are not covered means that there is something wrong in the code paths):

  • app/models/concerns/alegre_v2.rb: 41, 325, 326 and 327

Easy! The first line (41) is a TemporaryProjectMedia function that's just uncovered and I can add, and the 325-327 lines are because we don't check for an error in requests during deletes. Should be easy to mock, stand by!

DGaffney avatar Oct 16 '24 12:10 DGaffney