Desired?

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?

So how do you run this test?

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

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

Should we also make sure the user is logged in?

If we are going to require this, why not just drop the 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

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

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

I need to remove this unnecessary comment

proposal:

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

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

Looks good to me :)