Skip to content

Fix form redirection after bulk delete#99

Merged
Berdir merged 3 commits intomd-systems:pattern-config-entities-simplified-uifrom
juampynr:delete-submit-redirect
Dec 26, 2015
Merged

Fix form redirection after bulk delete#99
Berdir merged 3 commits intomd-systems:pattern-config-entities-simplified-uifrom
juampynr:delete-submit-redirect

Conversation

@juampynr
Copy link

The Bulk Delete form redirects to the Bulk Update form after performing the deletion. This pull request fixes it.

The Bulk Delete form was redirecting to the Bulk Update form
instead of back to Bulk Delete.
@Berdir
Copy link
Member

Berdir commented Dec 20, 2015

Nice find. Maybe we can extend the bulk delete test to to make sure we have a test for this?

@juampynr
Copy link
Author

@Berdir, when I looked at the test I saw the following:

    // 1. Test that deleting all the aliases, of any type, works.
    $this->generateAliases();
    $edit = array(
      'delete[all_aliases]' => TRUE,
    );
    $this->drupalPostForm('admin/config/search/path/delete_bulk', $edit, t('Delete aliases now!'));
    $this->assertText(t('All of your path aliases have been deleted.'));

The above checks the basics: that the form does the deletion. I don't feel right for adding a check to see that the form redirected where it should. I think that it is a matter of me understanding how far do we want to go with the testing coverage. Do we want to cover every line of logic? If that is the case, then my worry is that we may end up with such a huge test suite that it would be hard to maintain. I personally prefer to test bits of the module that are tricky to understand and prone to break. What do you think?

cc @EclipseGc

@Berdir
Copy link
Member

Berdir commented Dec 25, 2015

The fact that it was non-trivial enough for us to do it incorrectly is proof enough for me that it would make sense to have it covered by tests. We also have the assertion for the confirmation message which is equally trivial. And it just requires a single line. If it would be complicated to test, that could change things..

@juampynr
Copy link
Author

Fair enough. Adding...

@juampynr
Copy link
Author

Added!

Berdir added a commit that referenced this pull request Dec 26, 2015
Fix form redirection after bulk delete
@Berdir Berdir merged commit fa62702 into md-systems:pattern-config-entities-simplified-ui Dec 26, 2015
@Berdir
Copy link
Member

Berdir commented Dec 26, 2015

Thanks, merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants