Opened 5 years ago

Last modified 7 months ago

#12974 new Bug

Admindocs app introspection omits several model methods

Reported by: jabapyth Owned by: jabapyth
Component: contrib.admindocs Version: master
Severity: Normal Keywords: admin, docs, method, admindocs
Cc: danols@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The problem is that some methods are preprocessed (especially those with a @decorator) are ignored by admin docs, because they take no arguments as opposed to a single (self) argument. This patch changes a single character (from '==' to '<=').

Attachments (2)

admindocs_methods.diff (537 bytes) - added by jabapyth 5 years ago.
patch
admindocs_methods_trunk.diff (612 bytes) - added by jabapyth 5 years ago.
diff'd against the root

Download all attachments as: .zip

Change History (16)

Changed 5 years ago by jabapyth

patch

comment:1 Changed 5 years ago by jabapyth

  • Keywords admindocs added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by gvangool

  • Needs tests set

comment:3 Changed 5 years ago by russellm

  • Patch needs improvement set
  • Resolution set to worksforme
  • Status changed from new to closed

Firstly - please generate patches against the root of the source tree.

Secondly, due to the absence of a concrete test case (either in code, or in the problem description), I can't work out what problem it is that this patch is trying to solve.

Changed 5 years ago by jabapyth

diff'd against the root

comment:4 Changed 5 years ago by jabapyth

  • Resolution worksforme deleted
  • Status changed from closed to reopened

When admindocs displays methods for a model, it only displays ones w/ no arguments, or rather with only the (self) argument. The reason for this is no doubt to display only those methods which are valid for use in a template.

This patch is trying to solve the following problem:
Occasionally a model can have a method with no arguments, due to preprocessing @decorators or the use of the django.utils.curry function. These methods are then overlooked by admindocs, which checks for exactly one argument.
With this patch, it will check for fewer than or equal to one required argument.

The 'test' case that brought this problem to my attention was using that django-basic-apps blog app, whose Post model defines a get_absolute_url method. This method has a @permalink decorator before it, which results in Post.get_absolute_url having zero arguments. Given that the current code checks for exactly one argument, this method (which is totally valid for use in a template) is not displayed on the docs page.

Looking deeper into the code, I found that any get_absolute_url (even without a @permalink) gets curryd by the base model, and would therefore never be displayed by admindocs.

comment:5 Changed 5 years ago by jabapyth

  • Needs tests unset
  • Patch needs improvement unset

comment:6 Changed 5 years ago by russellm

  • Component changed from Documentation to django.contrib.admin
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Ok - problem now confirmed. For future reference: While I was eventually able to piece together an example that broke, it took some work. The best test case isn't a longer, more verbose verbal explanation -- it's a working sample of code that demonstrates the breakage, along with a short set of instructions for how to cause the breakage. For example:

Sample model:

class Thing(models.Model):
    name = models.CharField(max_length=100)
    
    @models.permalink
    def get_absolute_url(self):
        "Get the URL"
        return ('blog_detail', None, {})

    def button(self):
        "Push the button"
        pass

If you inspect /admin/doc/models/myapp.thing/ (the admin docs for the Thing model), button will be displayed, but get_absolute_url will not.

The best part is that this programatic test case can form the start of the test for your patch. Your patch doesn't have a test case, so the 'needs test case' flag still applies. The fact that this particularly feature doesn't have any tests at the moment doesn't give you a pass to not include tests -- it's an opportunity to add tests to make sure the feature will continue to work.

I've also turned on patch needs improvement -- At this point, the check for function arguments is redundant, since you can't have a function with a negative argument count.

comment:7 Changed 5 years ago by jabapyth

Could you explain the 'redundant' comment? The function must not have any required arguments (at least that is my impression of the desired behavior), and the check is therefore still necessary. Are you referring to some line other than the one that I modified?

comment:8 Changed 4 years ago by ramiro

  • Summary changed from Admin docs omits several methods for a Model to Admindocs app introspection omits several model methods

comment:9 Changed 4 years ago by lukeplant

  • Type set to Bug

comment:10 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:11 Changed 4 years ago by danols@…

  • Easy pickings unset
  • UI/UX unset

Please note that even if you remove @models.permlink from above example get_absolute_url still fails to be documented in the admin documentation. Further more functions that take no arguments that are template friendly also fail to be documented (see model definition below).

However this is not the reason I am adding to this ticket. I believe the assumption that admindoc should only document template friendly functions is flawed. For the admindoc app to be useful as a documentation portal for both the template designer and django programmer all functions should be included; template friendly functions can simply be differentiated visually.

Documenting all functions simply means removing len(inspect.getargspec(func)[0]) == 1 from the if statement on line 242. If needed I can submit a patch.

Thank you

class Example(models.Model):
    ### django native method
    def get_absolute_url(self):
        """ I'm not in the admin """
        pass
        
    def button(self):
        "I'm in the admin"
        pass
    
    ### extra model functions
    def get_next_order():
        """ I'm not in the admin """
        pass
    
    name = models.CharField(help_text='Name', max_length=20)

comment:12 Changed 4 years ago by Daniel Sokolowski <danols@…>

  • Cc danols@… added

(adding myself to cc)

comment:13 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:14 Changed 7 months ago by timo

  • Component changed from contrib.admin to contrib.admindocs
Note: See TracTickets for help on using tickets.
Back to Top