500 error in dashboard_activity_list_html (Fixes #256) #694

Merged
merged 9 commits into from Apr 17, 2013

Conversation

Projects
None yet
3 participants
Contributor

nigelbabu commented Mar 23, 2013

This does not return 500 anymore, which is great. But I've just run into interesting problems.

  • The tests render templates_legacy/activity_streams/activity_stream_items.html instead of templates/activity_streams/activity_stream_items.html
  • When I set an offset of 1, it prints <li class=\"load-less\"><a href=\"/packages?id=2048f54a-a259-457e-aa54-535ba77a57ba&amp;offset=-28\" class=\"btn btn-rounded\">Load less</a></li> which is from https://github.com/okfn/ckan/blob/master/ckan/templates/activity_streams/activity_stream_items.html#L5. Whatever this is meant to link to, I'm pretty certain it's wrong.

Fixes #256

Does anyone have insights on this?

Contributor

nigelbabu commented Mar 23, 2013

@tobes just answered the first question in another pull request.

@ghost ghost assigned tobes Mar 26, 2013

Contributor

tobes commented Mar 26, 2013

template issue

Contributor

tobes commented Mar 26, 2013

@nigelbabu

When I set an offset of 1, it prints

what does this mean? can you give me the url that triggers this issue

ckan/logic/action/get.py
@@ -2644,11 +2644,13 @@ def dashboard_activity_list_html(context, data_dict):
'''
activity_stream = dashboard_activity_list(context, data_dict)
+ model = context['model']
+ user_id = model.User.get(context['user']).id
@tobes

tobes Mar 26, 2013

Contributor

I think you can just replace these 2 lines with
user_id = c.userobj.id

this is the current user and saves the db call

Contributor

nigelbabu commented Mar 26, 2013

Try this url: http://127.0.0.1:5000/api/3/action/dashboard_activity_list_html?offset=1 Make sure you have at least two items in your activity stream.

ckan/logic/action/get.py
@@ -2644,11 +2644,13 @@ def dashboard_activity_list_html(context, data_dict):
'''
activity_stream = dashboard_activity_list(context, data_dict)
+ model = context['model']
+ user_id = model.User.get(context['user']).id
offset = int(data_dict.get('offset', 0))
extra_vars = {
'controller': 'dashboard',
@tobes

tobes Mar 26, 2013

Contributor

@nigelbabu this is the problem there is no dashboard controller - routes returns the dataset url when it can't find anything correct the correct controller needs to be supplied check the action as well. this may need some thought and possibly a rewrite of the logic and templates as it may not be compatible with the api calls

@nigelbabu

nigelbabu Apr 2, 2013

Contributor

Ran out of core time last sprint. I'll get on it this week.

@ghost ghost assigned seanh Apr 10, 2013

Contributor

seanh commented Apr 10, 2013

@tobes the issue was assigned to me, but the pull request was assigned to you! I'm taking a loot at this pr now anyway..

There's a shared test helper function for posting to the action api, which saves a lot of boilerplate: https://github.com/okfn/ckan/blob/master/ckan/tests/__init__.py#L411

This docstrings could be a little clearer, "works" is quite ambiguous. All this tests is that it returns successfully, Maybe ''Test that dashboard activity list html call doesn't crash.'' You could just rename the method to test_10_dashboard_activity_list_html_does_not_crash and then you wouldn't even need the docstring.

You need to get rid of this print statement. Checkout out ipdb and the ipdb nose plugin, they can be useful for debugging without putting print statements in

Contributor

seanh commented Apr 10, 2013

@nigelbabu Have you fixed the offset of 1 issue now?

This fix looks good to me I'll merge it once you respond to my minor comments above.

As a general comment your commit messages could be better, the later ones seem to have gotten more detailed than the earlier ones but still e.g. "Fix the duplicate dataset test failure" what duplicate test failure? It looks like you're assuming some knowledge there.

P.S. I realise now why there was no test for dashboard_activity_list_html, it's because it's part of the frontend.

@ghost ghost assigned nigelbabu Apr 10, 2013

[#256] Rename dashboard activity list test
* Rename dashboard activity list test for clarity.
* Remove extraneous print statement.
Contributor

nigelbabu commented Apr 17, 2013

@sean I've renamed the function and removed the print statement. I usually do use pdb along with tests, but occasionally, I get lazy and use print statements.

@ghost ghost assigned seanh Apr 17, 2013

@@ -4,7 +4,7 @@
{% if activities %}
<ul class="activity" data-module="activity-stream" data-module-more="{{ has_more }}" data-module-context="{{ controller }}" data-module-id="{{ id }}" data-module-offset="{{ offset }}">
{% if offset > 0 %}
- <li class="load-less"><a href="{{ h.url_for(controller=controller, action=action, id=id, offset=offset-30) }}" class="btn btn-rounded">{{ _('Load less') }}</a></li>
+ <li class="load-less"><a href="{{ h.url_for(controller=controller, action=action, id=id, offset=(offset-30 if offset-30 > 0 else 0)) }}" class="btn btn-rounded">{{ _('Load less') }}</a></li>
@seanh

seanh Apr 17, 2013

Contributor

@nigelbabu Did you fix the offset of 1 issue that you mentioned in the pull request description? I noticed you made this change to the offset parameter, which doesn't seem related to the original 500 error.

@nigelbabu

nigelbabu Apr 17, 2013

Contributor

This change is to fix parts of the offset issue.

@ghost ghost assigned nigelbabu Apr 17, 2013

- 'action': 'activity',
- 'id': data_dict['id'],
+ 'controller': 'user',
+ 'action': 'dashboard',
'offset': offset,
@nigelbabu

nigelbabu Apr 17, 2013

Contributor

This is the fix for the offset issue.

@ghost ghost assigned seanh Apr 17, 2013

Contributor

seanh commented Apr 17, 2013

Ok, I'm happy to merge this, I'm just waiting to see if the tests pass on travis after I pulled master into this branch

@seanh seanh merged commit ffcc901 into master Apr 17, 2013

1 check passed

default The Travis build passed
Details
Contributor

seanh commented Apr 17, 2013

Merged and emailed Adria for a 2.0 merge

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