Skip to content

Commit

Permalink
remove confirmation step in delete notifications
Browse files Browse the repository at this point in the history
  • Loading branch information
kennyevil committed Oct 25, 2023
1 parent 776c2a9 commit c24ba86
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 102 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,7 @@ class ResponsiblePersons::DeleteNotificationController < SubmitApplicationContro
before_action :validate_responsible_person
before_action :set_notification

def delete; end

def destroy
unless params.dig(:confirmation, :yes) == "yes"
@notification.errors.add(:confirmation, "Select yes if you want to delete this #{@notification.notification_complete? ? 'notification' : 'draft'}")
return render(:delete)
end

def delete
NotificationDeleteService.new(@notification, current_user).call

tab = @notification.notification_complete? ? "notified" : "incomplete"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module Operations
CHANGE_EMAIL_ADDRESS => 300, # 5 minutes
INVITE_USER => 300, # 5 minutes
UNLOCK => 300, # 5 minutes
DELETE_NOTIFICATION => 900, # 15 minutes
DELETE_NOTIFICATION => 30, # 30 seconds
SETUP_APP_AUTHENTICATION => 300, # 5 minutes
SETUP_SMS_AUTHENTICATION => 300, # 5 minutes
}.freeze
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,16 @@
<div class="govuk-grid-column-full govuk-!-margin-top-4">
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<% if @notification.archived? %>
<%= govukButton(text: "Unarchive this notification", href: responsible_person_notification_unarchive_path(@responsible_person, @notification), classes: "govuk-button--secondary") %>
<% else %>
<%= govukButton(text: "Archive this notification", href: responsible_person_notification_choose_archive_reason_path(@responsible_person, @notification, back_to: "notification"), classes: "govuk-button--secondary") %>
<% end %>
<% if @notification.can_be_deleted? %>
<%= govukButton(text: "Delete this notification", href: delete_responsible_person_delete_notification_path(@responsible_person, @notification), classes: "govuk-button--warning") %>
<% end %>
<div class="govuk-button-group">
<% if @notification.archived? %>
<%= link_to "Unarchive this notification", responsible_person_notification_unarchive_path(@responsible_person, @notification), class: "govuk-button govuk-button--secondary" %>
<% else %>
<%= link_to "Archive this notification", responsible_person_notification_choose_archive_reason_path(@responsible_person, @notification, back_to: "notification"), class: "govuk-button govuk-button--secondary" %>
<% end %>
<% if @notification.can_be_deleted? %>
<%= link_to "Delete this notification", delete_responsible_person_delete_notification_path(@responsible_person, @notification), class: "govuk-button govuk-button--warning" %>
<% end %>
</div>
</div>
</div>
<% unless @notification.can_be_deleted? %>
Expand Down
1 change: 0 additions & 1 deletion cosmetics-web/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,6 @@
resources :delete_notification, param: :reference_number, controller: "responsible_persons/delete_notification", only: [] do
member do
get :delete
delete :destroy
end
end
end
Expand Down
18 changes: 0 additions & 18 deletions cosmetics-web/spec/features/submit/notifications_deletion_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,6 @@
click_on draft_notification.product_name
click_on "Delete this draft"

expect(page).to have_h1("Do you want to delete this draft?")
expect(page).to have_text("The #{draft_notification.product_name} product notification draft will be deleted.")
click_button "Delete"

expect(page).to have_css("h2#error-summary-title", text: "There is a problem")
expect(page).to have_link("Select yes if you want to delete this draft", href: "#yes")
check "Yes"
click_button "Delete"

assert_text "#{draft_notification.product_name} notification deleted"
end

Expand All @@ -35,15 +26,6 @@
click_on "View #{notification.product_name}"
click_on "Delete this notification"

expect(page).to have_h1("Do you want to delete this notification?")
expect(page).to have_text("The #{notification.product_name} product notification will be deleted.")
click_button "Delete"

expect(page).to have_css("h2#error-summary-title", text: "There is a problem")
expect(page).to have_link("Select yes if you want to delete this notification", href: "#yes")
check "Yes"
click_button "Delete"

assert_text "#{notification.product_name} notification deleted"

log = NotificationDeleteLog.first
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,53 +16,26 @@
end

context "when deletion is confirmed" do
it "redirects" do
delete responsible_person_delete_notification_path(responsible_person, draft_notification, confirmation: { yes: "yes" })

expect(response.status).to eq 302
end

it "creates log record with current user" do
expect(NotificationDeleteLog.count).to eq 0

delete responsible_person_delete_notification_path(responsible_person, notification, confirmation: { yes: "yes" })
get delete_responsible_person_delete_notification_path(responsible_person, notification)

expect(NotificationDeleteLog.first.submit_user).to eq user
end

it "removes record" do
draft_notification
expect {
delete responsible_person_delete_notification_url(responsible_person, draft_notification, confirmation: { yes: "yes" })
get delete_responsible_person_delete_notification_path(responsible_person, notification)
}.to change(Notification.deleted, :count).from(0).to(1)
end
end

context "when deletion is not confirmed" do
it "renders the deletion page" do
delete responsible_person_delete_notification_path(responsible_person, draft_notification, confirmation: { yes: "0" })

expect(response.status).to eq 200
expect(response).to render_template("responsible_persons/delete_notification/delete")
end

it "does not create a notification delete log" do
expect {
delete responsible_person_delete_notification_path(responsible_person, notification, confirmation: { yes: "0" })
}.not_to change(NotificationDeleteLog, :count)
end

it "does not remove record" do
notification
expect {
delete responsible_person_delete_notification_url(responsible_person, notification, confirmation: { yes: "0" })
}.not_to change(Notification, :count)
end
end

context "when 2FA time passed", :with_2fa do
before do
get delete_responsible_person_delete_notification_url(responsible_person, draft_notification, confirmation: { yes: "yes" })
get delete_responsible_person_delete_notification_url(responsible_person, draft_notification)
post secondary_authentication_sms_url,
params: {
secondary_authentication_form: {
Expand All @@ -76,18 +49,18 @@

it "does not remove record" do
expect {
delete responsible_person_delete_notification_url(responsible_person, draft_notification, confirmation: { yes: "yes" })
get delete_responsible_person_delete_notification_url(responsible_person, draft_notification)
}.not_to change(Notification, :count)
end

it "redirects to 2FA" do
draft_notification
delete responsible_person_delete_notification_path(responsible_person, draft_notification, confirmation: { yes: "yes" })
get delete_responsible_person_delete_notification_path(responsible_person, draft_notification)
expect(response).to redirect_to("/two-factor/sms")
end

it "redirects info page to 2FA" do
get delete_responsible_person_delete_notification_path(responsible_person, draft_notification, confirmation: { yes: "yes" })
get delete_responsible_person_delete_notification_path(responsible_person, draft_notification)

expect(response.status).to be(302)
end
Expand All @@ -101,7 +74,7 @@

it "does not succeed" do
expect {
delete responsible_person_delete_notification_path(other_responsible_person, draft_notification)
get delete_responsible_person_delete_notification_path(other_responsible_person, draft_notification)
}.to raise_error(Pundit::NotAuthorizedError)
end
end
Expand Down

0 comments on commit c24ba86

Please sign in to comment.