• 
      

    Add a mixin for updating models with a ModelForm

    Review Request #7494 — Created July 6, 2015 and submitted

    Information

    Djblets
    release-0.9.x
    1cbef5d...

    Reviewers

    The UpdateFormMixin is a mixin for a WebAPIResource that allows a
    resource to specify a form class (which should be a subclass of
    django.forms.ModelForm) to use for creating and updating of model
    instances. This allows the resource that are very basic wrappers around
    models to have their logic further simplified.

    This mixin is required for allowing partial updating of models.
    Traditionally, a ModelForm can take an instance parameter
    specifying an already existing model instance to update, but will still
    require all required field to specified in the form data. This mixin
    takes care of that by extracting missing form data from the instance
    and inserting it into the form data in the proper format (such as
    setting it to be a list of primary keys in the case of a
    ModelMultipleChoiceField).

    This mixin also allows resources to specify special functions for
    parsing values out of the recieved form data. This is useful, e.g., for
    automatically converting human-readable values accepted by the API to
    the value the database expects for a ChoiceField.

    Used this in a Review Board API resource.

    Description From Last Updated

    This should live in djblets/webapi/resources/mixins/forms.py.

    chipx86chipx86

    Can you change Model to django.db.models.Model? After having played with the doc stuff a bit, I realized we need to …

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Similar here: django.forms.Form

    chipx86chipx86

    NotImplementedError isn't really right here. That'd be more for a function that should have been overridden. A better thing to …

    chipx86chipx86

    Can you also add a module-wide docstring? Even just a summary is fine. That will help it show up in …

    chipx86chipx86

    I believe this is django.db.models.Model.

    chipx86chipx86

    "non-None" seems weird. How about "%s must define a form_class attribute." ?

    chipx86chipx86

    Let's make sure this whole flow takes the request and all the kwargs from the caller and passes all the …

    chipx86chipx86
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          djblets/webapi/mixins.py
      
      
      
      Tool: Pyflakes
      Processed Files:
          djblets/webapi/mixins.py
      
      
    2. 
        
    brennie
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          djblets/webapi/mixins.py
      
      
      
      Tool: Pyflakes
      Processed Files:
          djblets/webapi/mixins.py
      
      
    2. 
        
    chipx86
    1. There's a ) missing in the first paragraph of your change description.

    2. djblets/webapi/mixins.py (Diff revision 2)
       
       
      Show all issues

      This should live in djblets/webapi/resources/mixins/forms.py.

    3. djblets/webapi/mixins.py (Diff revision 2)
       
       
      Show all issues

      Can you change Model to django.db.models.Model? After having played with the doc stuff a bit, I realized we need to really be using explicit module paths in these cases, so that linking will work well.

    4. djblets/webapi/mixins.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    5. djblets/webapi/mixins.py (Diff revision 2)
       
       
      Show all issues

      Similar here: django.forms.Form

    6. djblets/webapi/mixins.py (Diff revision 2)
       
       
      Show all issues

      NotImplementedError isn't really right here. That'd be more for a function that should have been overridden. A better thing to do is:

      assert self.form_class, '%s does not have ....' % ...
      
    7. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          djblets/webapi/resources/mixins/forms.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          djblets/webapi/resources/mixins/forms.py
      
      
    2. 
        
    chipx86
    1. 
        
    2. Show all issues

      Can you also add a module-wide docstring? Even just a summary is fine. That will help it show up in the docs. (Blank line after the docstring.)

      Speaking of which, can you add this to docs/djblets/coderef/index.rst?

    3. Show all issues

      I believe this is django.db.models.Model.

    4. Show all issues

      "non-None" seems weird. How about "%s must define a form_class attribute." ?

    5. 
        
    chipx86
    1. 
        
    2. Show all issues

      Let's make sure this whole flow takes the request and all the kwargs from the caller and passes all the way through the parse function. We used to not do this for field serialization, and regretted it. Sooner or later, something will need that data.

    3. 
        
    brennie
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          djblets/webapi/resources/mixins/forms.py
      
      Ignored Files:
          docs/djblets/coderef/index.rst
      
      
      
      Tool: Pyflakes
      Processed Files:
          djblets/webapi/resources/mixins/forms.py
      
      Ignored Files:
          docs/djblets/coderef/index.rst
      
      
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    brennie
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-0.9.x (c3e14b0)