Possibly wrong test

[ojii]

Working on #3516 I found a test that kept failing regardless what I tried. The test in question is cms.PluginstTestCase.test_remove_plugin_not_associated_to_page.

At https://github.com/divio/django-cms/blob/2fa7c67818df7e5379c19d1186dc012a2f2bde80/cms/tests/plugins.py#L540 it does a request to add_plugin with a POST parameter placeholder. However, this is wrong, as it should be placeholder_id. Thus it gets a 404 because it hits https://github.com/divio/django-cms/blob/2fa7c67818df7e5379c19d1186dc012a2f2bde80/cms/admin/placeholderadmin.py#L220-L221 but looking at the test name, this doesn't seem to be what it should be testing.

I've originally written the test, and back then PageAdmin wasn't allowed to edit any placeholders that are not associated with a page, and this test is there for that. See cfe8341. It however feels like this is simply wrong now.

Should this test be removed? Anyone know what's going on?

[evildmp]

I should think you are probably right. Also, "test_remove_plugin_not_associated_to_page" isn't a very good test name anyway. It's not clear to me what it is testing.

[ojii]

Historically it tested that PageAdmin.add_plugin cannot be used by requests attempting to add a plugin on to a placeholder not associated with a page, for security reasons (they had to use PlaceholderAdmin.add_plugin. So it should be called "test_add_plugin_not_associated_to_page_isnt_allowed_on_page_admin".

I could refactor the test to "test_not_providing_placeholder_id_to_add_plugin_makes_request_fail" which is what it tests in my branch on #3516

[yakky]

👍