Resolves #4985: Picklists should be filterable #5148
Conversation
|
|
||
| get requests_path(request), params: params | ||
|
|
||
| expect(response.body).to include('Print Unfulfilled Picklists (1)') |
There was a problem hiding this comment.
This is a bit of a sideways way of testing that the filters work - I'd expect to check that the response body includes the picklist that is returned and not the one that's filtered out.
There was a problem hiding this comment.
This test is explicitly meant to test that the value shown in the button text is correct, not that filters as a whole work. The filters working to filter the results shown within the app already worked, and so I'm not trying to test that since it's not part of the issue this PR is trying to solve.
However, having the results filtered in the downloaded PDF is part of this issue - I'm just not sure how to test that, as noted in the original PR comment.
There was a problem hiding this comment.
Can you get some more information out of this response? It's not really testing which data is being rendered, only that there's 1 of them.
|
Thanks for the feedback @dorner! I've simplified the test (filtering on status instead of partner organization) in a way that I think solves your concerns and also fixes some other issues I found. If you approve of the updates and my responses to your comments, I think the only remaining issue is if/how to test the PDF output, as discussed above. |
|
Re the PDF testing - did you look at https://github.com/rubyforgood/human-essentials/blob/d8fa236212a4114d962d4f7ac1002f42ba058c2e/lib/test_helpers/pdf_comparison_test_factory.rb ? |
|
Thanks @dorner, that file looks like it might be the right strategy for the most robust test of this PR. Unfortunately is looks like it is only set up to work with distribution PDFs, not picklists PDFs, and I don't think I'd be the right person to figure out how to adapt/copy that strategy for picklist PDFs. Since previous work on the picklist page didn't include tests that compared against the generated PDFs, I'm not sure if they're needed for this PR, but if they are perhaps we put this change on hold until someone more familiar with the codebase and RSpec can work on that. |
|
I think we do need to validate the results in the PDF. If you don't think you'll be able to do that kind of work, I'd rather bump the difficulty for this up to Intermediate and reopen it. |
|
Thank you for the solid start, @joepaolicelli! Per your wishes, I have re-"Help Wanted" the issue so we can get someone with the particular skills needed to work on it. |
- Generate the pdf and add to fixture - Rename content in PDFComparisonTestFactory to be more domain specific - Add compare_pdf to spec to compare the file to the
jonny5
force-pushed
the
4985-print-filtered-picklists
branch
from
953a54c to
d62f919
Compare
|
@dorner I added the PDF test if you want to take another look |
dorner
left a comment
dorner
left a comment
There was a problem hiding this comment.
Looks great - had one last suggestion.
|
|
||
| get requests_path(request), params: params | ||
|
|
||
| expect(response.body).to include('Print Unfulfilled Picklists (1)') |
There was a problem hiding this comment.
Can you get some more information out of this response? It's not really testing which data is being rendered, only that there's 1 of them.
|
@dorner updated spec to check for comments from request in body, that seem good? |
| get requests_path({ filters: { by_status: :started} }) | ||
|
|
||
| expect(response.body).to include('Print Unfulfilled Picklists (1)') | ||
| expect(response.body).to include(started_request.comments) |
There was a problem hiding this comment.
Please use the actual string that's expected - whenever possible, expected results should not be calculated.
jonny5
force-pushed
the
4985-print-filtered-picklists
branch
from
6348698 to
a3c66f7
Compare
awwaiid
left a comment
awwaiid
left a comment
There was a problem hiding this comment.
Functional review all checks out!
|
@joepaolicelli: Your PR |
5 participants
Resolves #4985.
Please note I have only intermittently worked with Ruby for the past several years, so apologies in advance for any mistakes due to that. The amount of code changed here is minimal, and I did my best to base it on similar code from elsewhere in the codebase. I ran bin/lint and no issues were detected.
Description
Applies filters to the printable "Unfulfilled Picklists" PDF, instead of always printing every request. Also updates the count shown on the "Print Unfulfilled Picklists".
Type of change
How Has This Been Tested?
I've added a test to ensure the "Print Unfulfilled Picklists" button shows the correct quantity when filters are applied. However, I was unable to determine how to add a test to check if the correct number of pages are created in the PDF, or if the testing framework was even set up to allow that. Instead, I manually tested several different configurations of filters to ensure that they led to only the expected filtered requests being including in the PDF.
Screenshots