Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

More powerful Inline Admin `extra` parameter (#19425) #576

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
7 participants

crodjer commented Dec 7, 2012

Right now, the number of extra inline forms in inline admins is
not that flexible. It should be possible to define a method
which can decide the number based on the context.

Issue in question: https://code.djangoproject.com/ticket/19425

@charettes charettes commented on an outdated diff Dec 7, 2012

django/contrib/admin/options.py
@@ -1467,6 +1467,14 @@ def media(self):
js.extend(['SelectBox.js', 'SelectFilter2.js'])
return forms.Media(js=[static('admin/js/%s' % url) for url in js])
+ def get_extra(self, request, obj=None, **kwargs):
+ """Get the number of extra inline forms needed"""
+ # extra = 3
+ if not isinstance(self.extra, int):
+ raise ImproperlyConfigured("'%s.extra' should be a integer."
+ % self.__class__.__name__)
@charettes

charettes Dec 7, 2012

Member

There's no need to move validation here, keep it in validation.py.

@charettes charettes commented on an outdated diff Dec 7, 2012

django/contrib/admin/options.py
@@ -1467,6 +1467,14 @@ def media(self):
js.extend(['SelectBox.js', 'SelectFilter2.js'])
return forms.Media(js=[static('admin/js/%s' % url) for url in js])
+ def get_extra(self, request, obj=None, **kwargs):
+ """Get the number of extra inline forms needed"""
+ # extra = 3
@charettes

charettes Dec 7, 2012

Member

We could remove this comment.

Member

charettes commented Dec 7, 2012

I've added some comments regarding the extra property validation.

Maybe I'm missing something here but I can't see where you've defined tests. There's an extra models and associated InlineAdmin subclass but no assertions that everything is working fine.

crodjer commented Dec 7, 2012

Sorry, I seemed to have missed staging the changes to tests.py. Also,
included your other suggestions.

Rohan Jain

On Fri, Dec 7, 2012 at 9:43 PM, Simon Charette notifications@github.comwrote:

I've added some comments regarding the extra property validation.

Maybe I'm missing something here but I can't see where you've defined
tests. There's an extra models and associated InlineAdmin subclass but no
assertions that everything is working fine.


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/576#issuecomment-11135524.

Member

charettes commented Dec 7, 2012

Looks better, you can also remove the validation in the get_extra method.

crodjer commented Dec 8, 2012

Okay. Thanks for pointing it out. Did a forced push to prevent stale
commits.

Rohan Jain

On Fri, Dec 7, 2012 at 11:19 PM, Simon Charette notifications@github.comwrote:

Looks better, you can also remove the validation in the get_extra method.


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/576#issuecomment-11139420.

@apollo13 apollo13 commented on an outdated diff Dec 30, 2012

tests/regressiontests/admin_inlines/tests.py
@@ -175,6 +176,20 @@ def test_custom_pk_shortcut(self):
self.assertContains(response, child2_shortcut)
+ def test_custom_get_extra_form(self):
+ bt_head = BinaryTree(name="Tree Head")
+ bt_head.save()
+ bt_child = BinaryTree(name="First Child", parent=bt_head)
+ bt_child.save()
+
+ inline_form = '<input type="hidden" name="binarytree_set-2-id" id="id_binarytree_set-2-id" />'
+ response = self.client.get('/admin/admin_inlines/binarytree/add/')
+ self.assertNotContains(response, inline_form)
+
+ response = self.client.get('/admin/admin_inlines/binarytree/' + str(bt_head.id) + '/')
@apollo13

apollo13 Dec 30, 2012

Owner

That str() call is most likely not a good idea on python3, using %d should work better.

@charettes charettes commented on an outdated diff Feb 5, 2013

django/contrib/admin/options.py
@@ -1493,7 +1497,7 @@ def get_formset(self, request, obj=None, **kwargs):
"fields": fields,
"exclude": exclude,
"formfield_callback": partial(self.formfield_for_dbfield, request=request),
- "extra": self.extra,
+ "extra": self.get_extra(request, obj, *kwargs),
@charettes

charettes Feb 5, 2013

Member

This should be **kwargs.

@charettes charettes commented on the diff Feb 5, 2013

docs/ref/contrib/admin/index.txt
@@ -1606,6 +1608,23 @@ The ``InlineModelAdmin`` class adds:
Returns a ``BaseInlineFormSet`` class for use in admin add/change views.
See the example for :class:`ModelAdmin.get_formsets`.
+.. method:: InlineModelAdmin.get_extra(self, request, obj=None, **kwargs)
@charettes

charettes Feb 5, 2013

Member

This needs a .. versionadded:: 1.6 clause.

Member

charettes commented Feb 5, 2013

Ok this needs four things before getting in.

  1. Fix the **kwargs issue;
  2. Add the versionadded clause;
  3. Add an entry in the 1.6 release note under minor features;
  4. Rebase against master.

crodjer commented Feb 6, 2013

Rebased and applied suggested fixes.

@charettes charettes commented on an outdated diff Feb 6, 2013

tests/regressiontests/admin_inlines/tests.py
@@ -190,6 +191,20 @@ def test_create_inlines_on_inherited_model(self):
self.assertEqual(Sighting.objects.filter(et__name='Martian').count(), 1)
+ def test_custom_get_extra_form(self):
+ bt_head = BinaryTree(name="Tree Head")
+ bt_head.save()
+ bt_child = BinaryTree(name="First Child", parent=bt_head)
+ bt_child.save()
+
+ inline_form = '<input type="hidden" name="binarytree_set-2-id" id="id_binarytree_set-2-id" />'
+ response = self.client.get('/admin/admin_inlines/binarytree/add/')
+ self.assertNotContains(response, inline_form)
+
+ response = self.client.get("/admin/admin_inlines/binarytree/%d/" %(bt_head.id))
@charettes

charettes Feb 6, 2013

Member

You can remove the parenthesis around bt_head.id.

Member

charettes commented Feb 6, 2013

You'll have to double-check your testcase -- It doesn't fail without the get_extra addition.

crodjer commented Feb 7, 2013

Updated the test case

@charettes charettes commented on an outdated diff Feb 8, 2013

docs/releases/1.6.txt
@@ -61,6 +61,9 @@ Minor features
:attr:`~django.core.management.BaseCommand.leave_locale_alone` internal
option. See :ref:`management-commands-and-locales` for more details.
+* ``InlineModelAdmin`` can override ``get_extra`` method to programmatically
@charettes

charettes Feb 8, 2013

Member

Could you change those to class and method references.

@charettes charettes commented on an outdated diff Feb 8, 2013

docs/ref/contrib/admin/index.txt
@@ -1618,6 +1620,25 @@ The ``InlineModelAdmin`` class adds:
Returns a ``BaseInlineFormSet`` class for use in admin add/change views.
See the example for :class:`ModelAdmin.get_formsets`.
+.. method:: InlineModelAdmin.get_extra(self, request, obj=None, **kwargs)
+
+ Returns the number of extra inline forms needed. By default, returns the
+ :attr:`InlineModelAdmin.extra` attribute.
+ Useful when you want to programmatically determine, the number of extra
+ inline forms should be shown in the interface, probably based on the
+ object passed as the keyword argument ``obj``. For example::
@charettes

charettes Feb 8, 2013

Member

You should revise the wording here, that Useful... sentence could be restructured.

crodjer commented Feb 12, 2013

Updated the docs.

crodjer added some commits Dec 7, 2012

@crodjer crodjer More powerful Inline Admin `extra` parameter (#19425)
Right now, the number of extra inline forms in inline admins is
not that flexible. It should be  possible to define a method
which can decide the number based on the context.

Signed-off-by: Rohan Jain <crodjer@gmail.com>
387be5e
@crodjer crodjer Add missed test for inline form #19425
Add the test case for verifying `get_extra` implementation
for binary tree admin.
Missed staging the file on previous commit.

Signed-off-by: Rohan Jain <crodjer@gmail.com>
d7c5483
@crodjer crodjer Inline admin `extra` option for back to validations #19425
Signed-off-by: Rohan Jain <crodjer@gmail.com>
a3d9e21
@crodjer crodjer Use string formatting instead of `str` for compatibility
Responding to comment: #576 (comment)

Signed-off-by: Rohan Jain <crodjer@gmail.com>
8799d7e
@crodjer crodjer Add documentation for `InlineModelAdmin.get_extra`
Ref: #19425
Signed-off-by: Rohan Jain <crodjer@gmail.com>
bf81657
@crodjer crodjer Fix `kwargs` syntax, use two asterisks
Signed-off-by: Rohan Jain <crodjer@gmail.com>
a805e77
@crodjer crodjer Add 1.6 release notes for `get_extra` parameter
Signed-off-by: Rohan Jain <crodjer@gmail.com>
40fcbfc
@crodjer crodjer Fix broken test case for `get_extra`
Use TOTAL_FORMS count. It'll remain 2 for a binary tree

Signed-off-by: Rohan Jain <crodjer@gmail.com>
4556ce3
@crodjer crodjer Fix updates to release notes and method documentation f0c491d

@ptone ptone commented on an outdated diff Mar 19, 2013

docs/ref/contrib/admin/index.txt
@@ -1574,6 +1574,8 @@ The ``InlineModelAdmin`` class adds:
The dynamic link will not appear if the number of currently displayed forms
exceeds ``max_num``, or if the user does not have JavaScript enabled.
+ Also see :meth:`InlineModelAdmin.get_extra` for advanced functionality
@ptone

ptone Mar 19, 2013

Member

"for advanced functionality" seems vague - may I suggest "if you need to customize the value" or similar, not a big deal.

@ptone ptone commented on an outdated diff Mar 19, 2013

docs/ref/contrib/admin/index.txt
+ :attr:`InlineModelAdmin.extra` attribute.
+
+ Here, you can programmatically determine the number of extra inline forms
+ which should be shown on the interface. This may be based on the object
+ passed as the keyword argument ``obj``. For example::
+
+ class BinaryTreeAdmin(admin.TabularInline):
+ model = BinaryTree
+
+ def get_extra(self, request, obj=None, **kwargs):
+ extra = 2
+ if obj:
+ return extra - obj.binarytree_set.count()
+ return extra
+
+.. versionadded:: 1.6
@ptone

ptone Mar 19, 2013

Member

related to @charettes comment above, this usually precedes the section

Contributor

areski commented May 19, 2013

Great patch, I tested it during the sprint djangocon eu and it worked for me.
when I rebase it I add a small conflict at the import django.core.exceptions. areski/django@f971398#L0R16

@crodjer crodjer Proper versionadded decleration
For the `get_extra` method in InlineModelAdmin

Signed-off-by: Rohan Jain <crodjer@gmail.com>
df5ac83

crodjer commented May 19, 2013

@ptone Added your suggestions in df5ac83

Sorry for responding so late to this thread.

Owner

timgraham commented May 30, 2013

merged in 36aecb1, thanks!

@timgraham timgraham closed this May 30, 2013

crodjer commented May 31, 2013

Hey, is this the way how all patches are merged in django? This has
happened with two of my patches already. However trivial this pull
request might have been, but this kills any motivation one will have
to contribute.

After investing any effort in a (small or large) patch, getting the
name in git log --author=<name> matters, which apparently is
insignificant here.

There is a proper way in git through which all the changes can be
combined into one, whilst not loosing any of the logs. Copy pasting a
diff into a separate commit is not at all the right way.

On Thu, May 30, 2013 at 11:40 PM, Tim Graham notifications@github.com wrote:

merged in 36aecb1, thanks!


Reply to this email directly or view it on GitHub.

Thanks
Rohan Jain

Contributor

areski commented May 31, 2013

Totally agree with @crodjer
The right way IMHO : when you don't agree on the patch comment on it until the patch is good for merging and then you should be able to accept the pull request and keep the original author. If after awhile there is no reactivity from the original author then provide your own fix. Everybody wants to see their contributions make their way to the awesome Django project.

Member

mjtamlyn commented May 31, 2013

I think one of the reasons it's often done this way is because each patch is required to be a single commit with a certain format of commit message. Whilst I admit this approach can put some contributors off, requiring that they rebase their work into a single commit with a certain format of commit message can also put people off. Historically when we were still on SVN, all commits were done in this fashion.

Also, core committers have a responsibility to ensure that the tests pass with your changes, so we have to pull everything in to our own dev environment and check the patch over. At this point, it's easier to fix any merge conflict issues and commit the patch ourselves. Your name will always be included in the commit message with some thanks.

Regarding the "name on the page" point - if you have not yet added yourself to AUTHORS please feel free to submit a PR and mention me in it and I'll merge it in for you.

Hope that helps.

crodjer commented May 31, 2013

On Fri, May 31, 2013 at 2:40 PM, Marc Tamlyn notifications@github.com wrote:

I think one of the reasons it's often done this way is because each patch is required to be a single commit with a > certain format of commit message. Whilst I admit this approach can put some contributors off, requiring that they > rebase their work into a single commit with a certain format of commit message can also put people off.
Historically when we were still on SVN, all commits were done in this fashion.

I understand making the changes into one commit. Git's interactive
rebase (git rebase -i) provide can be used do just that. I don't
make them into a single at every change, because of the off chance the
added (force pushed) change might not work. So, it is best to do a
interactive rebase just before merging the change. I would have been
happy to do that.

Also, core committers have a responsibility to ensure that the tests pass with your changes, so we have to pull
everything in to our own dev environment and check the patch over. At this point, it's easier to fix any merge
conflict issues and commit the patch ourselves. Your name will always be included in the commit message with >
some thanks.

Developers testing patches in their own dev environments is fine, but
I disagree with the need to author the patch by themselves because it
is "easier". Losing development history invalidates the use of an
advanced VCS to some extent. The actual development log and
documentation is not available anymore, which can be useful for future
reference.

Regarding the "name on the page" point - if you have not yet added yourself to AUTHORS please feel free to
submit a PR and mention me in it and I'll merge it in for you.

Thanks, I'll do that. But I think it will be awesome if it could be
this instead: git log --format="%aN <%aE>" | sort -u.

Hope that helps.

Yes, it does. I still have a few disagreements, but thanks for the
elaborate response.


Reply to this email directly or view it on GitHub.

Thanks
Rohan Jain

Contributor

areski commented Jun 3, 2013

There is an easy way to preserve the name of the original author with git.
You might want to use the following to merge the commit properly.
git merge --squash
git commit --author "Author Name email@address.com"
Other option before you push:
git commit --amend --author="Author Name email@address.com"

This will be a better practice to credit the community efforts and the core dev can add a comment with their name for further tracking needs (although we still have track for that)

crodjer commented Jun 4, 2013

Exactly. With squash, non of the documentation done in the commits is lost and
can be easily referred to later, in case that is needed. Even if the author
name isn't manually set, the logs are accessible in the project history
and not just under a github issue page of which you can't be sure if will
be accessible in future or not.

On 02:10 -0700 / 3 Jun, Areski Belaid wrote:

There is an easy way to preserve the name of the original committer with git.
You might want to use the following to merge the commit properly.
git merge --squash
git commit --author "Author Name email@address.com"
Other option before you push:
git commit --amend --author="Author Name email@address.com"

This will be a better practice to credit the community efforts and the core dev can add a comment with their name for further tracking needs (although we still have track for that)


Reply to this email directly or view it on GitHub:
#576 (comment)

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