#215 Measure the frequency of visit of a hub
Merged 7 years ago by skrzepto. Opened 7 years ago by skrzepto.
skrzepto/fedora-hubs issue-185  into  develop

Visit Counter
skrzepto • 7 years ago  
file modified
+28
@@ -554,6 +554,34 @@ 

      return hub

  

  

+ @app.route('/visit/<visited_hub>/', methods=['GET', 'POST'])

+ @app.route('/visit/<visited_hub>', methods=['GET', 'POST'])

+ @login_required

+ def increment_counter(visited_hub):

+     nickname = flask.g.auth.nickname

+ 

+     if str(visited_hub) != str(nickname):

+         try:

+             vc = hubs.models.VisitCounter.get_or_create(session=session,

+                                                         username=nickname,

+                                                         visited_hub=visited_hub)

+         except ValueError:  # this should never trip

+             # flask will 405 if visited_hub is blank

+             # @login_required forces flask.g.auth to be sets

+             flask.abort(404)

+         if flask.request.method == 'POST':

+             vc.increment_visits(session=session,

+                                 username=nickname,

+                                 visited_hub=visited_hub)

+ 

+             return flask.jsonify({'count': vc.count})

+ 

+         elif flask.request.method == 'GET':

+             return flask.jsonify({'count': vc.count})

+     else:

+         return flask.abort(403)

+ 

+ 

  def get_widget(session, hub, idx):

      """ Utility shorthand to get a widget and 404 if not found. """

      try:

file modified
+59
@@ -419,3 +419,62 @@ 

  

              session.commit()

          return self

+ 

+ 

+ class VisitCounter(BASE):

+     __tablename__ = 'visit_counter'

+     count = sa.Column(sa.Integer, default=0, nullable=False)

+ 

+     visited_hub = sa.Column(sa.String(50),

+                        sa.ForeignKey('hubs.name'),

+                        primary_key=True)

+ 

+     username = sa.Column(sa.Text,

+                         sa.ForeignKey('users.username'),

+                         primary_key=True)

+ 

+     user = relation("User", backref=backref('visit_counters', cascade="all, delete, delete-orphan"))

+     hub = relation("Hub", backref=backref('visit_counters', cascade="all, delete, delete-orphan"))

+ 

+     @classmethod

+     def by_username(cls, session, username):

+         return session.query(cls).filter_by(username=username).all()

+ 

+     @classmethod

+     def get_visits_by_username_hub(cls, session, username, visited_hub):

+         return session.query(cls).filter_by(username=username, visited_hub=visited_hub).first()

+ 

+     @classmethod

+     def increment_visits(cls, session, username, visited_hub):

+         row = cls.get_or_create(session, username=username,

+                                 visited_hub=visited_hub)

+         row.count += 1

+         session.commit()

+ 

+     @classmethod

+     def _does_hub_exist(cls, session, hub=''):

+         return session.query(Hub).filter_by(name=hub).first() is not None

+ 

+     @classmethod

+     def _does_user_exist(cls, session, user=''):

+         return session.query(User).filter_by(username=user).first() is not None

+ 

+     @classmethod

+     def get_or_create(cls, session, username, visited_hub):

+         if not username:

+             raise ValueError("Must provide an username, not %r" % username)

+ 

+         if not visited_hub:

+             raise ValueError("Must provide an hub, not %r" % visited_hub)

+ 

+         if not cls._does_hub_exist(session, hub=visited_hub) \

+                 or not cls._does_user_exist(session, user=username):

+             raise ValueError("Must provide a hub/user that exists")

+ 

+         self = cls.get_visits_by_username_hub(session, username=username,

+                                               visited_hub=visited_hub)

+         if not self:

+             self = cls(username=username, visited_hub=visited_hub)

+             session.add(self)

+             session.commit()

+         return self

file modified
+24
@@ -274,6 +274,30 @@ 

    });

  }

  

+ function visit_counter() {

+   if (window.performance.navigation.type == 0) {

+     var username = '{{ g.auth.nickname }}'

+     var visited_hub = '{{ hub.name }}'

+     if(username != null && username != '' && username != visited_hub) {

+       url_str = '/visit/' + visited_hub;

+ 

+       $.ajax({

+         method: "POST",

+         url: url_str,

+         dataType: 'html',

+         success: function(html) {

+           console.log('Success: incrementing counter')

+         },

+         error: function() {

+           console.log('Error: incrementing counter');

+         },

+       });

+     }

+   }

+ }

+ 

+ visit_counter()

+ 

  function setup_widgets(widgets) {

    var all_widgets = [{% for widget in hub.widgets %}'{{ widget.idx }}',{% endfor %}];

    if (widgets == undefined) {

@@ -285,5 +285,46 @@ 

              self.assertEqual(result.status_code, 302)

              self.assertEqual(urlparse(result.location).path, '/ralph/')

  

+     def test_hub_visit_counter_logged_in(self):

+         user = tests.FakeAuthorization('ralph')

+         with tests.auth_set(app, user):

+             url = '/visit/decause'

+             result = self.app.get(url)

+             self.assertEqual(json.loads(result.data), {"count": 0})

+ 

+             result = self.app.post(url)

+             self.assertEqual(json.loads(result.data), {"count": 1})

+ 

+             # accessing my hub shouldn't increment the count

+             url = 'visit/ralph'

+             result = self.app.post(url)

+             self.assertEqual(result.status_code, 403)

+ 

+             # visiting no hub while logged should throw a 405

+             url = 'visit/'

+             result = self.app.post(url)

+             self.assertEqual(result.status_code, 405)

+ 

+             # visiting a hub that doesn't exist should 404

+             url = 'visit/hub-does-not-exist'

+             result = self.app.post(url)

+             self.assertEqual(result.status_code, 404)

+ 

+     @unittest.skip("Ajax calls don't seem to work in unittests ")

+     def test_hub_vist_counter_logged_in_2(self):

+         user = tests.FakeAuthorization('ralph')

+         with tests.auth_set(app, user):

+             url = '/visit/decause'

+             result = self.app.get(url)

+             self.assertEqual(result.data, '0')

+ 

+             url = '/decause'

+             result = self.app.get(url, follow_redirects=True)

+ 

+             url = '/visit/decause'

+             result = self.app.get(url)

+             self.assertEqual(result.data, '1')

+ 

+ 

  if __name__ == '__main__':

      unittest.main()

file modified
+70
@@ -88,3 +88,73 @@ 

  

          hub = hubs.models.Hub.get(self.session, username)

          self.assertIsNone(hub)

+ 

+     def test_visit_counter(self):

+         username = 'ralph'

+         hub = 'decause'

+         # Make sure the table is empty of data

+         vc = hubs.models.VisitCounter.get_visits_by_username_hub(

+             session=self.session, username=username, visited_hub=hub)

+         self.assertIsNone(vc)

+ 

+         # Insert a new counter row

+         vc = hubs.models.VisitCounter.get_or_create(

+             session=self.session, username=username, visited_hub=hub)

+         # Make sure its init to 0

+         self.assertEqual(vc.count, 0)

+ 

+         # Increment counter and make sure its 1

+         hubs.models.VisitCounter.increment_visits(

+             session=self.session, username=username, visited_hub=hub)

+         self.assertEqual(vc.count, 1)

+ 

+         # Delete the counter make sure the hub/user is still arround

+         vc = hubs.models.VisitCounter.get_or_create(

+             session=self.session, username=username, visited_hub=hub)

+         self.session.delete(vc)

+         hub_obj = hubs.models.Hub.get(self.session, username)

+         self.assertIsNotNone(hub_obj)

+         user_obj = hubs.models.User.get(session=self.session,

+                                         username=username)

+         self.assertIsNotNone(user_obj)

+ 

+         # Delete hub and make sure the visit counter is 0

+         vc = hubs.models.VisitCounter.get_visits_by_username_hub(

+             session=self.session, username=username, visited_hub=hub)

+         self.session.delete(hub_obj)

+         self.assertIsNone(vc)

+         user_obj = hubs.models.User.get(session=self.session,

+                                         username=username)

+         self.assertIsNotNone(user_obj)

+ 

+     def test_visit_counter_does_not_exist(self):

+         username = 'ralph'

+         hub = 'does-not-exist'

+         self.assertRaises(ValueError,

+                           hubs.models.VisitCounter.get_or_create,

+                           session=self.session,

+                           username=username,

+                           visited_hub=hub)

+ 

+         username = 'does-not-exist'

+         hub = 'ralph'

+         # Make sure the table is empty of data

+         self.assertRaises(ValueError,

+                           hubs.models.VisitCounter.get_or_create,

+                           session=self.session,

+                           username=username,

+                           visited_hub=hub)

+ 

+         username = 'does-not-exist'

+         hub = 'does-not-exist'

+         # Make sure the table is empty of data

+         self.assertRaises(ValueError,

+                           hubs.models.VisitCounter.get_or_create,

+                           session=self.session,

+                           username=username,

+                           visited_hub=hub)

+ 

+ 

+ 

+ 

+ 

file modified
+2 -1
@@ -237,7 +237,8 @@ 

  widget = hubs.models.Widget(plugin='dummy', index=3, left=True)

  hub.widgets.append(widget)

  

- 

+ vc = hubs.models.VisitCounter().get_or_create(session, 'ralph', 'mrichard')

+ hubs.models.VisitCounter().increment_visits(session, 'ralph', 'mrichard')

  hub.subscribe(session, hubs.models.User.by_username(session, 'ralph'), 'owner')

  hub.subscribe(session, hubs.models.User.by_username(session, 'abompard'), 'owner')

  hub.subscribe(session, hubs.models.User.by_username(session, 'lmacken'), 'owner')

issue #185
Keep a counter of what hubs the user accessed so we can use it to suggest ordering bookmarks.

rebased

7 years ago

Should visited_hub be its name or its id?

hubs doesn't have an id though?

It was something leftover while debugging flask_oidc. I'll uncomment it

I wasn't sure anymore :)

Do we want CSRF here?

I think we might need to. This implementation seems easy to abuse

This seems to be able to raise exceptions, should we catch them?

What kind of exceptions do you think?

I'm not seeing it used in the new lines, was it missing from before?

I was trying to get if __main___ to run the test suite here. Changed my mind on that

I manually tested it with this same order. I'm not sure we auto unittest if the js ajax can't be called

rebased

7 years ago

Mentioned in a previous comment. Should we add CSRF here?

Should we also make sure the user is logged in?

1 new commit added

  • added login requrired to increment visit counter and make sure the user requesting the increment is the same user
7 years ago

If we are going to require this, why not just drop the <username> part of the url?

you make a good point, it becomes redundant. I'll update that

Even though you are not using this value, I would suggest to change it to return json

1 new commit added

  • removing redundant variable username and just using visited_hub
7 years ago

rebased

7 years ago

rebased

7 years ago

More complex implementation is done. Only increments the counter when its not a page refresh or page redirection from history. Also, added some security so its harder to abuse.

How can this be triggered with the @login_required above?

Because if that's the case, we have a bug :)

So what happens when this raises an exception?

Couple of question but this is looking quite nice :)

I guess i can't :P I was doing defensive programming which isn't good. I'll remove the redundant try statement.

It's good sometime but here if we have a bug we definitively want to know about it as there will be more things broken

ohh you mean when the username and hub are none

https://pagure.io/fedora-hubs/pull-request/215#2_39

hmm I think I should raise a 404 here if it does throw an exception

:thumbsup: for me

1 new commit added

  • removing the check if user is logged in because @login_required is there, also surrounded get_or_create in try statement even though it should never except
7 years ago

So 405 if the visited_hub is '' which makes sense this then the URL is invalid.

But what if I do a GET to /visit/<foo_bar_hubs_that_does_not_exist/ will it get created? Should it?

looks like it just creates it. Guess the foreign key isn't as restrictive as i thought

1 new commit added

  • adding tests and fixing where try/except goes
7 years ago

I need to remove this unnecessary comment

proposal:

return session.query(User).filter_by(username=user).first() is None

1 new commit added

  • convering does hub/user exist to one liners
7 years ago

I notice that I double not the booleans in _does_user/hub_exist maybe i should convert the name to _does_user/hub_not_exist and remove the not, thoughts?

not a blocker for me, up to you

rebased

7 years ago

Pull-Request has been merged by skrzepto

7 years ago