[#36, #37] Rework routing and use session to store current active request V2 #64
Conversation
There was a problem hiding this comment.
Overall looks great – couple of minor comments/suggestions.
I'm not really sure why we have the explicit Submission::UNQUEUED, as we never persist a Submission in that state.
I think it would be worth adding some class documentation to the lifecycle of a Submission (but feel free to do this in a separate PR – doesn't need to block this really.
|
|
||
| has_one :foi_request, dependent: :destroy | ||
|
|
||
| validates :state, presence: true | ||
|
|
||
| def queue | ||
| update(state: QUEUED) |
There was a problem hiding this comment.
This should have some unit tests
|
|
||
| def new; end | ||
|
|
||
| def create | ||
| redirect_to sent_foi_request_path | ||
| if @submission.queue |
There was a problem hiding this comment.
At this point, clicking "Send now" on the preview page results in:
- Returning to
/foi/request/new - A queued submission being created
Again, the specs don't fail, so I assume a case of #63.
There was a problem hiding this comment.
I can see this gets resolved in 0f8f255 – I don't know if this is just a case of swapping the commit order or maybe tweaking this commit. Its not that big a deal, but in principle all commits should work and pass specs.
There was a problem hiding this comment.
Huh strange, thought I double checked this. Happy to switch the ordering of the commits when merging.
There was a problem hiding this comment.
IDK if it was a caching issue – I did clear caches and restart server etc.
Up to you :)
app/models/foi_request.rb
Outdated
|
|
||
| scope :queued, lambda { | ||
| left_joins(:submission). | ||
| where.not(submissions: { state: [nil, Submission::UNQUEUED] }) |
There was a problem hiding this comment.
If I was writing this, I'd have probably written:
- where.not(submissions: { state: [nil, Submission::UNQUEUED] })
+ where(submissions: { state: Submission::QUEUED })Could you just go over why we want to assume queued submissions are anything other than "unqueued" submissions, or submissions with no state?
There was a problem hiding this comment.
In theory, if everything is works well, the submission will only be in the QUEUED state for seconds before being sent on. So I guess I'm trying to think ahead here, this scope should return requests which have been queued or sent or what ever a later state would be. Basically anything apart from the initial state so to confirmation page can be shown.
Equally with UNQUEUED I know its not being used yet but we need want a state which will represent that the user has submitted the request but queuing failed for some reason, EG Redis down, so it can be retried.
spec/models/submission_spec.rb
Outdated
| expect { submission.queue }.to change(submission, :state). | ||
| to(Submission::QUEUED) | ||
| end | ||
| end |
There was a problem hiding this comment.
I'd probably test that it gets persisted as well, since in use you're likely to call queue and not follow it with a save.
so maybe:
it 'persists the change' do
expect { submission.queue }.to change(submission, :persisted?).
to(true)
end
In preperation of changing to a singlar resource for the Requests which would result in the loss of the index route. Instead we are replacing it here with the `/foi` route.
Makes it clearer what is happening in the controller.
Less duplication making things easier to change and to test. We need to change the controller `:id` param to `:request_id`. This has been done by adding a new resources block in the routes as changing it on the original resources block also effect the params of sub-resources.
Which is how we're going to retrieve the request to be displayed.
This completes switching over to singular resource routes.
This prevents the ID param in the URL from being changed to access other requests.
This isn't needed as we're using the session to store the request ID now.
Submission isn't going to be present by default so this allows more flexibility in our specs which will be important for testing `FoiRequest.unqueued` and `FoiRequest.queued` scopes which will be implemented soon.
Although this has no real change yet this will means the request stored the session won't be returned once it has been submitted - after the submission state is changed.
This saves a submission record with state `queued` for the current request. This is where we'll create the background job. As a result of the previous commit, this means the request in the session isn't editable due to it not being returned by `FoiRequest.unqueued` anymore.
This is the post submission action so its safe to assume the request should be queued by now.
gbp
force-pushed
the
36-37-routing-session-rework-v2
branch
from
b153c53 to
e6364cf
Compare
Closes #36
Closes #37
Instead of storing the currently active request in a URL ID param we are now storing in the session and switched to singular resource routes.
Comments taken #60 and acted upon except splitting this into two PRs as that would mean #37 (specify this checklist) wouldn't be complete.
Have created a new issue #63 relating to integration testing which would of caught the broken commits.