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

Include plugin data in comment serialization, to obtain data for hearing visualization #168

Merged
merged 4 commits into from
Apr 15, 2016

Conversation

Rikuoja
Copy link
Contributor

@Rikuoja Rikuoja commented Apr 12, 2016

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0008%) to 98.358% when pulling 75f69cc on include-plugin-data into adaeb3c on master.

@akx
Copy link
Contributor

akx commented Apr 12, 2016

I don't think this is a good idea, or at the very least it should be parametrized. Plugin data is potentially very large, and downloading all of it for every request (even those for the regular UI) is probably not actually desired.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 98.317% when pulling c16f8bc on include-plugin-data into dc066ec on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 98.317% when pulling c16f8bc on include-plugin-data into dc066ec on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 98.275% when pulling dc5ccfe on include-plugin-data into dc066ec on master.

@akx
Copy link
Contributor

akx commented Apr 15, 2016

Missing tests for both new features added.

filtered_queryset = queryset.filter(**{queryset.model.parent_field: self.get_comment_parent_id()})
request = self.get_serializer_context().get('request', None)
if request:
auth_code = request.GET.get('auth_code', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

The request arg should probably be called authorization_code too.

How about using some sort of actual DRF filtering mechanism instead of hard-coding only one filter though?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 98.323% when pulling 0faae22 on include-plugin-data into dc066ec on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.387% when pulling 62836cf on include-plugin-data into dc066ec on master.

comment_data = get_comment_data(authorization_code="foo6")
url = get_hearing_detail_url(default_hearing.id, 'comments')
api_client.post(url, comment_data)
data = get_data_from_response(api_client.get(url+'?authorization_code=foo6'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Client.get() accepts the query string as a second parameter:

api_client.get(url, {"authorization_code": "foo6"})

@akx
Copy link
Contributor

akx commented Apr 15, 2016

Can you add a test that ensures that plugin data is not included when you don't pass in the include=plugin_data parameter?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 98.396% when pulling 06ef800 on include-plugin-data into dc066ec on master.

@akx
Copy link
Contributor

akx commented Apr 15, 2016

Let's do this! Thanks.

@akx akx merged commit a5a2584 into master Apr 15, 2016
@akx akx removed the in progress label Apr 15, 2016
@Rikuoja Rikuoja deleted the include-plugin-data branch July 20, 2017 13:20
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.

3 participants