Refactor Requests controller#5563
Conversation
Moving view instances to a View Object.
This was raising a bullet error: GET /donations AVOID eager loading detected Donation => [:product_drive_participant] Remove from your query: .includes([:product_drive_participant])
Moving view instances to a View Object to keep the controller clean. Add coverage for business logic
add coverage for Request model view
stefannibrasil
force-pushed
the
refactor-requests-controller-5557
branch
from
d3dc2a5 to
1f08698
Compare
| @@ -31,7 +31,6 @@ def from_params(params:, organization:, helpers:) | |||
| .includes(:storage_location, | |||
| :donation_site, | |||
| :product_drive, | |||
There was a problem hiding this comment.
I am happy to remove this change from this PR. I got here by looking at other view objects and saw the bullet's warning: 8089be6cb25bfff90506e634fc486dbaf6454e9b
dorner
left a comment
dorner
left a comment
There was a problem hiding this comment.
Love the intention! Had a couple of comments about the details.
| end | ||
|
|
||
| def location | ||
| StorageLocation.find_by(id: default_storage_location) |
There was a problem hiding this comment.
This isn't memoized, unlike the instance variable approach. Have you validated that this isn't called more than once?
The other methods also aren't memoized, but once loaded into memory Rails should be smart and not make more DB calls.
There was a problem hiding this comment.
Good call! Working on this.
There was a problem hiding this comment.
Memoized the methods that make queries and can be memoized in 257f971
| end | ||
| end | ||
|
|
||
| def unfulfilled_requests_count |
There was a problem hiding this comment.
Same comment as above - please validate that methods are either memoized, only called once, or would not result in additional DB calls.
| expect(request.custom_units).to be_truthy | ||
|
|
||
| Flipper.disable(:enable_packs) | ||
| end |
There was a problem hiding this comment.
There are no unit tests for any of the other methods?
There was a problem hiding this comment.
The reason why was because they were mostly accessing associations and it looks like testing rails internals at that point. So I focused on testing the methods that had business logic in them. But I added coverage for the remaining methods in 57a830d
| expect(requests.selected_partner).to eq("A Local Partner") | ||
| expect(requests.selected_status).to eq("pending") | ||
| end | ||
| end |
There was a problem hiding this comment.
Same reason as above... added full coverage in 2cfa660
To prevent performance degradation, `View::RequestInfo` memoizes expensive operations, especially DB calls. To make that happen, the object needs to be a regular class and not a Data object, which is immutable.
To prevent performance degradation, `View::Requests` memoizes expensive operations, especially DB calls. To make that happen, the object needs to be a regular class and not a Data object, which is immutable.
|
I create an issue for a flaky spec I encountered: #5573 |
2 participants
Resolves #5557
Description
In #5543, I added one more variable to this controller. I didn't want to refactor the controller since the PR was already too big and it's a good practice to refactor in a separate stream of work.
This PR moves instance variables used in the views to View objects. This keeps the controller slim and it's more maintainable to add methods to the view objects from now on.
Type of change
How Has This Been Tested?
Screenshots
Short walk trough of requests page:
requests.refactor.480.mov