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/#8 Add support to download documents #253

Merged

Conversation

olvr-me
Copy link
Contributor

@olvr-me olvr-me commented Nov 5, 2024

Hello @VMelnalksnis,

I came across issue #8 in your library and thought adding download functionality might be a useful enhancement, so I gave it a try and submitted a PR.

I wasn’t sure if you generally welcome PR contributions from others, so please feel free to give any feedback or decline the PR if it doesn’t align with your vision - this is your project, and I completely respect your direction.

I referred to the endpoint and functionality described here: Paperless API - Downloading Documents. I’m fairly new to Paperless, so I hope I’ve understood everything correctly. I added test cases to your IntegrationTest project, and they all passed successfully. I incorporated the download functionality into the existing create-test flow, where the document is downloaded and the ContentType, FileName, and content are checked against expected values. I used the presence of "lorem ipsum" in both the filename and content as validation to ensure everything is working as expected.

Since this is my first open-source contribution, I’m happy to receive any feedback you may have.

Thank you for creating/maintaining this project and considering my contribution!

Copy link

cocogitto-bot bot commented Nov 5, 2024

❌ Found 0 compliant commit and 7 non-compliant commits in 8c90362...d74a818.

Commit 8c90362 by @olvr-me is not conform to the conventional commit specification :

  • message: Add DS_Store to gitignore
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | Add DS_Store to gitignore
          |    ^---
          |
          = expected scope or type_separator
    

Commit a16423d by @olvr-me is not conform to the conventional commit specification :

  • message: First tests to implement downloading a file (WIP)
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | First tests to implement downloading a file (WIP)
          |      ^---
          |
          = expected scope or type_separator
    

Commit a3b8184 by @olvr-me is not conform to the conventional commit specification :

  • message: Fixed wrong routes and using
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Fixed wrong routes and using
          |      ^---
          |
          = expected scope or type_separator
    

Commit 31b870c by @olvr-me is not conform to the conventional commit specification :

  • message: Added data class for document content
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Added data class for document content
          |      ^---
          |
          = expected scope or type_separator
    

Commit fe7d1df by @olvr-me is not conform to the conventional commit specification :

  • message: Fix warning
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | Fix warning
          |    ^---
          |
          = expected scope or type_separator
    

Commit 5439c5e by @olvr-me is not conform to the conventional commit specification :

  • message: Use new class to also return fileName and ContentType of document
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | Use new class to also return fileName and ContentType of document
          |    ^---
          |
          = expected scope or type_separator
    

Commit d74a818 by @olvr-me is not conform to the conventional commit specification :

  • message: Added parameter in test for better readability
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Added parameter in test for better readability
          |      ^---
          |
          = expected scope or type_separator
    

@olvr-me olvr-me force-pushed the feature/#8_Download_Documents branch from d74a818 to e6c1360 Compare November 5, 2024 20:01
@VMelnalksnis
Copy link
Owner

Hi @olvr-me, I went through the changes and had some comments, if something's not clear or you disagree with something let me know. From the documentation it looks like preview and thumbnail should work the same, so maybe you could also add methods for that as part of this PR. The only thing that looks different is the request URI, so you could create a private method Task<DocumentContent> ContentCore(Uri requestUri, CancellationToken cancellationToken) that all the public methods use.

@olvr-me
Copy link
Contributor Author

olvr-me commented Nov 6, 2024

Thank you so much for the quick and detailed feedback!
I really appreciate the time you took to review and provide detailed comments. I agree with all your suggestions and aim to implement these changes by the end of this week.

There was just one comment I wasn’t entirely clear on, so I’ve left a follow-up question there for you when you have a moment.

@olvr-me
Copy link
Contributor Author

olvr-me commented Nov 9, 2024

Hi @VMelnalksnis,

I’ve implemented the suggested changes—thanks again for your feedback! I also had three follow-up questions, which I left in comments at the relevant changes. If you have a moment to look over those, I’d love to hear your thoughts.

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.16%. Comparing base (fafc89b) to head (846fbd8).
Report is 2 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...lksnis.PaperlessDotNet/Documents/DocumentClient.cs 90.90% 2 Missing ⚠️
...ksnis.PaperlessDotNet/Documents/DocumentContent.cs 88.88% 1 Missing ⚠️
source/VMelnalksnis.PaperlessDotNet/Routes.cs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
- Coverage   92.42%   92.16%   -0.27%     
==========================================
  Files          29       30       +1     
  Lines         449      485      +36     
  Branches       55       55              
==========================================
+ Hits          415      447      +32     
- Misses         19       23       +4     
  Partials       15       15              

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

@VMelnalksnis
Copy link
Owner

I'll take a look (again) on how to correctly setup actions with secrets. I can't seem to be able to get it working both for myself when working with branches, and for others working with forks.

@olvr-me
Copy link
Contributor Author

olvr-me commented Nov 11, 2024

Alright, no worries if you’re busy—there’s no pressure to respond right away.
Thank you very much for your time!

@VMelnalksnis
Copy link
Owner

VMelnalksnis commented Nov 29, 2024

@olvr-me Sorry it took so long, but actions should be working now if you rebase your branch. I see that there were some warnings that should be fixed though https://github.com/VMelnalksnis/PaperlessDotNet/actions/runs/11784782522/job/32844179014?pr=253#step:7:1269

@olvr-me olvr-me force-pushed the feature/#8_Download_Documents branch from 4643398 to 717cb97 Compare November 30, 2024 14:17
@VMelnalksnis VMelnalksnis linked an issue Nov 30, 2024 that may be closed by this pull request
@VMelnalksnis
Copy link
Owner

Looks good to me, can ignore the missing code coverage. I merge the commits from PRs as is into master, so since it's ready to be merged, could you squash the WIP commits? I think all the changes releated to document download can be a single feat commit, the .gitignore can stay separate.

@olvr-me olvr-me force-pushed the feature/#8_Download_Documents branch from 717cb97 to 846fbd8 Compare November 30, 2024 14:34
@olvr-me
Copy link
Contributor Author

olvr-me commented Nov 30, 2024

Hi @VMelnalksnis,

No worries about taking your time to respond - I just squashed the commits as you requested.

@VMelnalksnis VMelnalksnis merged commit 846fbd8 into VMelnalksnis:master Nov 30, 2024
3 of 5 checks passed
@olvr-me
Copy link
Contributor Author

olvr-me commented Nov 30, 2024

I just noticed that I hadn't removed the unnecessary using directive. Since I don't have ReSharper installed, I didn't see that warning before pushing. Sorry about that.

@VMelnalksnis
Copy link
Owner

Yeah, I must have done something wrong with the PR checks, it checked master instead of your branch. No worries, I'll fix it in my next PR.

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.

Add methods for document download API
2 participants