Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update admin.py #467

Closed
wants to merge 12 commits into from
Closed

Update admin.py #467

wants to merge 12 commits into from

Conversation

ozeranskii
Copy link

@ozeranskii ozeranskii commented Oct 29, 2018

add extra_context to history_form_view

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov-io
Copy link

codecov-io commented Oct 29, 2018

Codecov Report

Merging #467 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #467      +/-   ##
=========================================
+ Coverage   97.09%   97.1%   +<.01%     
=========================================
  Files          15      15              
  Lines         689     690       +1     
  Branches       96      96              
=========================================
+ Hits          669     670       +1     
  Misses         10      10              
  Partials       10      10
Impacted Files Coverage Δ
simple_history/admin.py 98% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3183611...3c26d8a. Read the comment docs.

@rossmechanic
Copy link
Collaborator

@ozeranskiy can you add some more context about this change? Also, add the change to CHANGES.rst and add your name to AUTHORS.rst

Sergey added 2 commits October 29, 2018 17:57
@ozeranskii
Copy link
Author

ozeranskii commented Oct 29, 2018

@ozeranskiy can you add some more context about this change? Also, add the change to CHANGES.rst and add your name to AUTHORS.rst

Done.
Usage example:

class SimpleHistoryAdminMixin(SimpleHistoryAdmin):
    def history_form_view(self, request, object_id, version_id, extra_kwargs=None):
        extra_kwargs = extra_kwargs or {}
        extra_kwargs['obj'] = 'My data'
        return super(SimpleHistoryAdminMixin, self).history_form_view(
            request, object_id, version_id, extra_context=extra_kwargs
        )

This is necessary if you need to pass custom parameters to the template "simple_history/object_history_form.html"

@@ -107,7 +107,8 @@ def response_change(self, request, obj):
return super(SimpleHistoryAdmin, self).response_change(
request, obj)

def history_form_view(self, request, object_id, version_id):
def history_form_view(self, request, object_id, version_id,
extra_context=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra_context is None by default, so you shouldn't have to add it here. Try to add separate tests where extra_context has an actual value and passes through

@rossmechanic
Copy link
Collaborator

Hey @ozeranskiy are you still working on this or want me to take it over?

@ozeranskii
Copy link
Author

Hey @ozeranskiy are you still working on this or want me to take it over?

In the near future I can not write a test. Can you accept the current state?

@barm
Copy link
Collaborator

barm commented Nov 12, 2018

I can take it over and add some additional tests this week. I'm not a fan of merge now and add tests later because it's easy to lose track of that work. Thanks for your contribution!

@ozeranskii
Copy link
Author

I can take it over and add some additional tests this week. I'm not a fan of merge now and add tests later because it's easy to lose track of that work. Thanks for your contribution!

ok, thanks you.

@barm barm mentioned this pull request Dec 9, 2018
9 tasks
@barm
Copy link
Collaborator

barm commented Dec 9, 2018

Opened #499 so I'll close this. Thanks again!

@barm barm closed this Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants