[MKT-575] Prevent accidental deletion in points #310
Conversation
| respond_to do |format| | ||
| format.html { redirect_to projects_path, notice: "Project was successfully destroyed." } | ||
| if @project.title.strip.eql?(params.dig(:project, :title)&.strip) |
There was a problem hiding this comment.
Hey @JuanVqz , Out of curiosity, why are we stripping here? should it not be exact match?
There was a problem hiding this comment.
I noticed that sometimes when editing the project title, it left a space at the end of the title. it might be confusing if you don't realize that when deleting a project.
Of course, we could normalize the title when saving it (strip it) and remove it here.
what do you think?
There was a problem hiding this comment.
I do not have a strong reason as to why choose 1 approach over the other. So I would say, lets keep what you have already implemented. Thanks for clarifying.
| it "does not delete the project" do | ||
| expect { | ||
| delete :destroy, params: {id: project.id} | ||
| }.not_to change(Project, :count) | ||
| end |
There was a problem hiding this comment.
Hey @JuanVqz , can we also add a spec for testing that it does not delete when we pass the title of the project but it is not the right title?
There was a problem hiding this comment.
I have added the spec since it was not something of a big change.
There was a problem hiding this comment.
Thanks! I've added that test as feature 👍
https://github.com/fastruby/points/pull/310/files#diff-b4cf077b77d7765559bc95291a6e424d712c59271a5d6bcc7eb79b2cd3726e87R59
|
The PR looks good from code review perspective. |
|
QA looks good! |
Jira Ticket
https://ombulabs.atlassian.net/browse/MKT-575
Fixes #258
Motivation / Context
Prevent accidental deletion in points
QA / Testing Instructions
Delete a project
Screenshots:
I will abide by the code of conduct.