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.

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

Add 'Delete All' button to web UI #367

Closed
wants to merge 5 commits into from
Closed

Add 'Delete All' button to web UI #367

wants to merge 5 commits into from

Conversation

ivandenysov
Copy link

@ivandenysov ivandenysov commented Feb 7, 2019

Close #357

I had to to put delete_all action above show action so it would get matched first.
I could have put it as last action if I used HTTP method POST instead of GET. But I assume that there's a reason why POST is not used for regular delete form. So I followed the same approach.
Or we can leave it like that. Maybe it's just me who is used to order all actions by where they belong in CRUD acronym 😅

@ivandenysov
Copy link
Author

Method registered has 26 lines of code (exceeds 25 allowed). Consider refactoring

Can somebody silence this CodeClimate error? 🙂

@mhenrixon mhenrixon self-assigned this Feb 8, 2019
Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

Thank you for contributing. The change itself looks good, could we have a test for it as well?

@ivandenysov
Copy link
Author

@mhenrixon added integration test for /unique_digests/delete_all endpoint

@mhenrixon
Copy link
Owner

Thanks @john-denisov, I fixed the style violations in #370 and will merge that as soon as the build passes.

@mhenrixon mhenrixon closed this Feb 10, 2019
@mhenrixon
Copy link
Owner

Closed with #370

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants