Shouldn't PlaceholderAdmin rather be a mixin class? #2709

Closed
jrief opened this Issue Feb 25, 2014 · 7 comments

Comments

Projects
None yet
3 participants
Contributor

jrief commented Feb 25, 2014

I just cam across the FrontendEditableAdmin, which is defined as a mixin class for customer classes derived from admin.ModelAdmin.
However, PlaceholderAdmin is derived from admin.ModelAdmin declaring an IS-A relationship. This, IMO makes is harder for reusage and also is not consistent regarding FrontendEditableAdmin.
Is there any good reason why this has to be so?

Contributor

digi604 commented Feb 25, 2014

nope... would accept a PR

Contributor

digi604 commented Feb 25, 2014

but may rise some backwards compatibility issues....

@digi604 digi604 added this to the Some Day milestone Feb 25, 2014

Contributor

jrief commented Feb 25, 2014

But maybe these backwards compatibility issues can be handled by a deprecation warning until version 3.1 or 3.2.
So, you also agree, that a mixin class in that context is cleaner.

Contributor

jrief commented Feb 25, 2014

I just released PR #2728 where PlaceholderAdmin was degraded to a mixin class. It now behaves like FrontendEditableAdmin.
Unfortunately a DeprecationWarning does not work here. I tried to check for the class type in PlaceholderAdmin.__new__ but its too late there. During registration Django complains if the class is not derived from admin.ModelAdmin; this is before __new__ would be called. Anyway, in my dev environment it works.

A much more serious problem is, that one Selenium tests fails:

raceback (most recent call last):
  File "django-cms/cms/tests/frontend.py", line 278, in test_copy_to_from_clipboard
    clipboard = self.driver.find_element_by_css_selector('.cms_clipboard')
  File "selenium/webdriver/remote/webdriver.py", line 365, in find_element_by_css_selector
    return self.find_element(by=By.CSS_SELECTOR, value=css_selector)
  File "selenium/webdriver/remote/webdriver.py", line 681, in find_element
    {'using': by, 'value': value})['value']
  File "selenium/webdriver/remote/webdriver.py", line 164, in execute
    self.error_handler.check_response(response)
  File "selenium/webdriver/remote/errorhandler.py", line 164, in check_response
    raise exception_class(message, screen, stacktrace)
NoSuchElementException: Message: u'Unable to locate element: {"method":"css selector","selector":".cms_clipboard"}' ; Stacktrace:
    at FirefoxDriver.prototype.findElementInternal_ (file:///var/folders/00/h9kwh1z91jj5fg68xm1vnb2m0000gn/T/tmpsszpOI/extensions/fxdriver@googlecode.com/components/driver_component.js:8904)
    at fxdriver.Timer.prototype.setTimeout/<.notify (file:///var/folders/00/h9kwh1z91jj5fg68xm1vnb2m0000gn/T/tmpsszpOI/extensions/fxdriver@googlecode.com/components/driver_component.js:396)

I stucked here. Any idea, why this test fails?

Contributor

yakky commented Feb 25, 2014

Probably the simpler option is to create a PlaceholderAdminMixin class which is the mixin and PlaceholderAdmin which implements it with a ModelAdmin (how java of me!)
This could probably avoided using some magic metaclass trick, but developers will need to change the code anyway, so it'd better to enforce a better naming.
And I'd rename FrontendEditableAdmin to FrontendEditableAdminMixin in 3.0 (without deprecation)

Contributor

jrief commented Feb 25, 2014

But then you have two classes for the same functionality, which IMO is really, really WET.
But renaming FrontendEditableAdmin to FrontendEditableAdminMixin probably is a good idea.
I'll try in Thursday.

Contributor

yakky commented Feb 26, 2014

The "concrete" class is meant to be legacy implementation that will be dropped in 3.1 (or whenever we decide to drop it).

@jrief jrief referenced this issue Feb 27, 2014

Merged

Fix issue 2709 #2740

@jrief jrief closed this Feb 27, 2014

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