Edit all data doc backend - #48433
Conversation
|
This is looking great! 🥳 Left some small comments inline |
davidsbailey
left a comment
davidsbailey
left a comment
There was a problem hiding this comment.
I got part way through reviewing, and was hoping to poke around at it. but the page does not appear to be loading any contents. I just created at least one data doc via /data_docs/new. am I on the right url?
Oh for now that's all it's meant to show! The frontend will be in a separate PR, I'm just doing what I did with the edit data doc PR where there's just some text saying this is the right page. I could add a list element like the index page to temporarily show all created docs until the frontend PR is merged if that sounds good? |
Oh I see - it does say it is the backend PR! this approach seems fine for now, maybe we can discuss the merits of this approach in standup sometime. |
| @data_doc.destroy! | ||
| render(status: :ok, plain: "Destroyed #{@data_doc.key}") | ||
| rescue | ||
| render(status: :not_acceptable, plain: @data_doc.errors) |
There was a problem hiding this comment.
this is what I wanted to experiment with -- what happens if I try to delete a doc that does not exist? I'm not 100% sure we'll hit this codepath, or if we'll have already rendered here:
I like the idea of rescuing known errors and returning a helpful message to the user, but only in cases we anticipate and where we know it was the client's fault. if there is another kind of error, it is important for us to return a 5xx, which is the default when an exception is not rescued. this will make sure it shows up as an error in HoneyBadger.
For now I'd say this looks good to merge, but let's revisit this as soon as we have a frontend to try deleting things (including things which do not exist).
There was a problem hiding this comment.
thank you for the feedback! I think you're right that line 65 will never be reached, so I think I'll fix it now for consistency with this being the backend PR!
There was a problem hiding this comment.
(deleted my last comment because it was in the wrong place)
Good call re :not_found here! I like that approach (not rescuing errors in the destroy method)
| assert_raise ActiveRecord::RecordNotFound do | ||
| data_doc_to_delete.reload | ||
| end | ||
| end |
There was a problem hiding this comment.
oh, I just realized that a unit test here would be a great way to test out what happens when you try to delete a nonexistent entry. I'm not sure this is super critical, so I'm also fine with you waiting until you have the frontend PR and testing that manually.
There was a problem hiding this comment.
will do! I'll add it now to keep the backend changes in this PR!
Yes that sounds good! I'd love some suggestions on how to best approach this! |
Ditto –– I like vertical slicing and I'm not sure how to always do that in cases like these when PRs get big. |
| get '/s/csp9-2020/lockable/1(*all)', to: redirect(path: '/s/csp9-2020/lessons/9%{all}') | ||
| get '/s/csp10-2020/lockable/1(*all)', to: redirect(path: '/s/csp10-2020/lessons/14%{all}') | ||
|
|
||
| resources :data_docs, only: [:new, :create, :edit, :update, :show, :index], param: :key |
There was a problem hiding this comment.
I feel like we've achieved a badge using all the routes 🥇
3 participants
This is the backend part of this Jira ticket for adding an editing interface for the list of all Data Docs, allowing an editor to add a new Data Doc, edit an existing Data Doc, or delete an existing Data Doc.
To see where the edit_all table will be:
Links
Jira ticket: here
Spec: requirement #3 here
Testing story
Local testing to ensure the addition of these files does not disrupt the current DataDoc creation and viewing behavior. Backend tests were also added for these changes.
Follow-up work
Adding the edit_all page frontend
PR Checklist: