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.

Sign up for GitHub

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

Jump to bottom

Feature of having with_user_activity=True parameter when using User Activity Feed (User Stream) url #468

Merged
merged 8 commits into from
Nov 21, 2020
Merged

Feature of having with_user_activity=True parameter when using User Activity Feed (User Stream) url #468

merged 8 commits into from
Nov 21, 2020

Conversation

ehsabd
Copy link
Contributor

@ehsabd ehsabd commented Sep 27, 2020

I liked to have a feature that I can get the user own activity by passing with_user_activity=True through query parameter like /activity/feed/json/with_user_activity=true.

So I added the items function to UserActivityMixin. Take a look at the code and tell me your comments about that or this feature.

@ehsabd
Update feeds.rst
8e790aa 
Add the method of using ``?with_user_activity=True`` in user activity feed to include user own actions.
@ehsabd
added as query string in UserActivityFeed
17e11e8
@coveralls
Copy link

Coverage Status

Coverage decreased (-95.1%) to 0.0% when pulling 17e11e8 on ehsabd:master into e7a919c on justquick:master.

@coveralls
Copy link

coveralls commented Sep 27, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 95.187% when pulling d412967 on ehsabd:master into e7a919c on justquick:master.

@ehsabd
Copy link
Contributor Author

ehsabd commented Sep 28, 2020 •
edited
Loading

I guess the change I've made, while works for Json activity feed breaks Atom activity feed. So perhaps I should change another function or create a Mixin for including with_user_activity as a kwarg when calling the stream.
Another idea is that I create a get_stream_kwargs() function in the abstract class and implement it accordingly.

@ehsabd
Copy link
Contributor Author

ehsabd commented Sep 29, 2020

I have updated the code. I added get_stream_kwargs as an abstract method in AbstractActivityStream and changed the code in the serialize method of JSONActivityFeed that gets the items to make use of get_stream_kwargs.

@ehsabd
Copy link
Contributor Author

ehsabd commented Sep 29, 2020 •
edited
Loading

It seemed none of the solutions above fit. I finally created a mixin (StreamKwargsMixin) that I only use with UserJSONActivityFeed this prevents breaking any other classes that inherit UserActivityMixin or JSONActivityFeed.

auvipy
auvipy requested changes Oct 13, 2020
View reviewed changes
Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

also, the mixin class needed to be tested

actstream/feeds.py Outdated
@@ -242,6 +242,11 @@ def get_object(self, request, content_type_id, object_id):
def get_stream(self):
return any_stream

class StreamKwargsMixin(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

explicit inheritance from the object is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah there is no need to do that. But I followed the pattern of other classes in feeds.py like AbstractActivityStream, ModelActivityMixin, etc.

Also I will add tests for StreamKwargsMixin

auvipy
auvipy requested changes Oct 31, 2020
View reviewed changes
Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looking for unit test

@ehsabd
Copy link
Contributor Author

ehsabd commented Nov 1, 2020

looking for unit test

I'm working on it. I hope I can provide it soon.

@ehsabd
- Fix assertAllIn in tests/base.py to work with json as well
d412967 
- Add query_string param to `capture` in tests/base.py
- Add test for json feed and json feed + with_user_activity=true (UserJSONActivityFeed inherits StreamKwargsMixin)
@ehsabd
Copy link
Contributor Author

ehsabd commented Nov 4, 2020

I've added the related tests to test_feeds.py. Indeed the json feed were not tested. So I've added a test for that and also a test with with_user_activity=true.

@ehsabd
Copy link
Contributor Author

ehsabd commented Nov 16, 2020

looking for unit test

Unit tests added. Can u take alook?

auvipy
auvipy reviewed Nov 17, 2020
View reviewed changes
docs/source/feeds.rst
@@ -26,6 +26,14 @@ Shows user stream for currently logged in user.
/activity/feed/atom/
/activity/feed/json/

If you want to include the user own activity, the optional parameter ``with_user_activity`` can be passed to the user stream as a query string parameter:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am willing to know if this is part of activity stream specification?

Copy link
Owner

Choose a reason for hiding this comment

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

meh

@justquick justquick merged commit bd11b7e into justquick:master Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auvipy auvipy auvipy requested changes

@justquick justquick justquick left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ehsabd @coveralls @justquick @auvipy