-
Notifications
You must be signed in to change notification settings - Fork 43
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
Popular links delete #2276
Popular links delete #2276
Conversation
7ff31bc
to
664df29
Compare
My first PR! Happy for any feedback at this point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Just a few minor comments.
link_to("Preview (opens in new tab)", preview_homepage_path(@latest_popular_links), rel:"noreferrer noopener", target:"_blank", class: "govuk-link") | ||
] | ||
link_to("Preview (opens in new tab)", preview_homepage_path(@latest_popular_links), rel:"noreferrer noopener", target:"_blank", class: "govuk-link"), | ||
link_to("Delete draft", confirm_destroy_popular_links_path( @latest_popular_links), class: "govuk-link gem-link--destructive") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove space before @latest_popular_links
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can still fix this one, right?
22cb175
to
b35dc5b
Compare
Add delete functionality and confirm destroy method Functional and integration tests added for delete mechanism Routes for 'confirm delete' page and actual destroy action
b35dc5b
to
e18f134
Compare
Destroy and confirm destroy functionality PopularLinksEdition.can_delete? method to test if edition can be deleted Tests for confirmation page and cancel delete
e18f134
to
bdbc344
Compare
Tests for: Show previously published edition Successful and unsuccessful delete from DB Application error message was being used twice, so extracted out to own method.
Added an extra check to ensure that there is no way to delete a published edition. To prevent issues with multiple users accessing the same edition.
Link renamed to 'Delete draft' Application error message makes it clear that it was a draft that failed to be deleted not an edition 'Can't delete published edition' message updated to be more helpful Confirmation and success messages reworded to be clear it is a draft that will be/has been deleted
bdbc344
to
fbad5d2
Compare
…e of error message method Fixed incorrect test title Fixup! Add missing test and extracted error message method Rename of error message method Removed content block that doesn't exist in mainstream
c01d865
to
51592ab
Compare
3c84406
to
d7e3605
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of very minor comments.
link_to("Preview (opens in new tab)", preview_homepage_path(@latest_popular_links), rel:"noreferrer noopener", target:"_blank", class: "govuk-link") | ||
] | ||
link_to("Preview (opens in new tab)", preview_homepage_path(@latest_popular_links), rel:"noreferrer noopener", target:"_blank", class: "govuk-link"), | ||
link_to("Delete draft", confirm_destroy_popular_links_path( @latest_popular_links), class: "govuk-link gem-link--destructive") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can still fix this one, right?
Improved test - more descriptive name and added assert for the error message Test improvements - more accurate names and extra assert Renamed tests to more explanatory names Added test to check alert and redirect if delete from DB fails with an exception Divided tests into 3 contexts - 'draft', 'published' and 'database errors' Moved context blocks 'destroy' and 'confirm destroy' to after 'publish' Moved the test for deletion failure to #destroy #draft context block Added redirect assert to test for can't delete published edition
d7e3605
to
1681ad9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM—good work!
Create delete mechanism for 'edit' page
Trello
Follow these steps if you are doing a Rails upgrade.