Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Popular links delete #2276

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Popular links delete #2276

merged 7 commits into from
Aug 14, 2024

Conversation

ellohez
Copy link
Contributor

@ellohez ellohez commented Aug 7, 2024 •
edited
Loading

Create delete mechanism for 'edit' page

  • Added a 'Delete draft' link to the sidebar.
  • Added a confirmation page for deleting the draft.
  • Added Delete button and Cancel link on the confirmation page.
  • Ensured that a Published edition cannot be deleted, either from the GUI or from entering the URL.
  • Delete from database hooked up to the final Delete button.

Trello

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@ellohez ellohez requested a review from mtaylorgds August 7, 2024 12:10
@ellohez
Copy link
Contributor Author

ellohez commented Aug 7, 2024

My first PR! Happy for any feedback at this point.

@alphagov alphagov deleted a comment from SyedAliTw Aug 7, 2024
mtaylorgds
mtaylorgds reviewed Aug 7, 2024
View reviewed changes
Copy link
Contributor

@mtaylorgds mtaylorgds left a 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.

app/controllers/homepage_controller.rb Show resolved Hide resolved
app/views/homepage/popular_links/_sidebar.html.erb Outdated
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")
Copy link
Contributor

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.

Copy link
Contributor

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?

app/views/homepage/popular_links/_sidebar.html.erb Show resolved Hide resolved
app/views/homepage/popular_links/confirm_destroy.html.erb Outdated Show resolved Hide resolved
test/functional/homepage_controller_test.rb Outdated Show resolved Hide resolved
test/functional/homepage_controller_test.rb Outdated Show resolved Hide resolved
test/functional/homepage_controller_test.rb Outdated Show resolved Hide resolved
test/integration/homepage_popular_links_test.rb Outdated Show resolved Hide resolved
test/integration/homepage_popular_links_test.rb Outdated Show resolved Hide resolved
@ellohez
Outline for destroy method and link
30c8e11 
Add delete functionality and confirm destroy method
Functional and integration tests added for delete mechanism
Routes for 'confirm delete' page and actual destroy action
@ellohez
Confirmation page created - removed delete page and links
833943a 
Destroy and confirm destroy functionality
PopularLinksEdition.can_delete? method to test if edition can be deleted
Tests for confirmation page and cancel delete
@ellohez
Destroy tests added and extracted error message out
256da1b 
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.
@ellohez
Add check if the edition can be deleted before deletion
8445f5c 
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.
@ellohez
Desk check changes - messages and labels made more consistent
87a1eaf 
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
@ellohez
PR Quick fixes, test title, removed incorrect content block and renam…
f0d6ace 
…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
mtaylorgds
mtaylorgds reviewed Aug 13, 2024
View reviewed changes
Copy link
Contributor

@mtaylorgds mtaylorgds left a 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.

app/views/homepage/popular_links/_sidebar.html.erb Outdated
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")
Copy link
Contributor

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?

test/functional/homepage_controller_test.rb Show resolved Hide resolved
@ellohez
PR - Test improvements and fixes
1681ad9 
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
mtaylorgds
mtaylorgds approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@mtaylorgds mtaylorgds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM—good work!

@ellohez ellohez merged commit 4515e05 into main Aug 14, 2024
12 checks passed
@ellohez ellohez deleted the popular-links-delete branch August 14, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syed-ali-tw syed-ali-tw syed-ali-tw left review comments

@mtaylorgds mtaylorgds mtaylorgds approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ellohez @syed-ali-tw @mtaylorgds