Should it be "Allows changing the format..." in the description?

After reading this description I'm still not clear how to use this. Is GET /api/0/user/ralph/activity/stats?format=timestamp right, and if so, does that produce the first sample response?

Looks like the code expects a value of "1" or "true" for grouped, so it might be good to document that here.

My guess is the date is in the format YYYY-MM-DD, but I think it would be good to document that since all sorts of date formats are used :disappointed:

This could probably be its own function

This is a readability nitpick, but httpcode will never have any value other than 200 as far as I can tell (everything else is raising an exception with an http code). It's clearer to just set it explicitly to 200 here and drop httpcode

I know Pagure isn't Python 3 yet, but it'd be better to use the function print() so there's one less thing to fix later

Is there a reason this call isn't down closer to where project is used? It's confusing to find a call and not see its result used anywhere nearby.

Never mind, I see it's in a loop now that I open the whole file

Same comment about using the print function as above. I'm also curious about what prompted this change.

This question may be answered elsewhere as I continue my review, but how will this work with private issues/comments? I can imagine a scenario where a security issue is reported as a private issue, but an attacker is able to read some information about the issue via the activity log. Based on the API docs it looks like it might just be the title, but that can be enough when it comes to security problems.

Can we use https://docs.python.org/2/library/string.html#formatstrings? It's new in Python 2.6 so support shouldn't be a problem. If nothing else, there's a lot fewer s characters dangling around: desc = '{user} {verb} {project}#{obj_id}'

You can keep the dictionary and do desc.format(**arg) if you like.

I also wonder if it would be better to attach this behaviour to the PagureLog model itself so the output of str(some_log_entry) gave me the human-readable description?

GET /api/0/user/ralph/activity/stats?format=timestamp is correct yes.

Mandatory arguments are basically in the URL while optional are as ?foo=...&bar=..

The first output sample is without timestamp, timestamp format is the second example

Agreed

Type should be boolean not string indeed

Sure

sure

Well, it's modifying both activities and js_act so not sure I agree

The logger isn't correctly configured in a git hook, so it's just as easy to us prints (+ it's consistent with our other hooks)

private issues aren't exposed on the API unless you are authenticated and are allowed to view the issue.

But the markdown processor might need to be adjusted for this indeed (no only for this feature)

I know the formatstring but I'd rather be consistent and so far I've used the '%' approach.

I do link the str(log) idea though, that will require some change to the DB though.
Do you want this here or in a different pull-request?

Do you have objections to moving all these new functions to a stats module within lib?

This might be a personal preference thing, but I typically expect __init__.py to do some importing and other setup for a package, but that's about it. Finding most of lib inside __init__.py was very surprising, and dealing with 1k+ modules quickly becomes painful.

I'd like to see everything moved out of this __init__.py into modules within lib eventually. What are your feelings on that?

Or maybe the module should be activity_log and include https://pagure.io/pagure/pull-request/1499#5_15

On the one hand it feels odd to remove activity log entries when a project is deleted, but on the other hand I can see how it'd be problematic to have log entries for projects/issues/PRs that got deleted.

What is in lib is what interacts with the DB, I'd rather keep all or most DB interaction in one place and so far __init__.py has been that place.

That and it would break the str(log) idea

Out of curiosity, why do issues have a uid column and what is it for when it also has an id column? I notice Project doesn't have this uid column.

Why does this have an underscore at the end? I'm not familiar with this convention.

Can we remove the commented out fields?

id might not be unique (it wouldn't happen with a regular use of pagure but if people mess with the git repo for issues/requests it could), uid are :)

uid are what is used as file name when dumping the JSON representation of a PR or an Issue (they are the only two object with an uid) in their respective git repos.

because type is a function in python type('foo') and I see no point in overriding it :)

should yes

Ah okay. My preference is to use type since this is namespaced and if someone wants to use the builtin they can do so explicitly with __builtins__. That's my preference, though, and if it's not appealing to you I won't quibble about it.

if type_ is really bugging you, then I'd rather move to something like log_type or so to avoid the entire problem :)

It would be very helpful to have a detailed comment for each of these regex with some examples of what will and won't match. That way when someone comes across it they don't have to pull out the manual each time.

For example, I had to look up that (?<!\w) is a negative lookbehind assertion that matches if the current position of the string is not proceeded by \w (which I had to look up to see matches any alphanumeric character and the underscore character and is locale-aware).

examples are (for me) in the unit-tests, that's even the primary reason why I added the tests (also because that regex is a bit of a PITA so I needed a way to check if it works)

Adjusted for:

EXPLICIT_LINK_RE = r'(?<!\w)'\  # Ensure we catch the motif from the start
'(fork[s]?/)?'\  # See if there is a `forks/` at the start
'([a-zA-Z0-9_-]*?/)?'\ # See if we have a `user/`
'([a-zA-Z0-9_-]*?/)?'\  # See if we have a `namespace/`
'([a-zA-Z0-9_-]+)'\  # Get the last part `project`
'#(?P<id>[0-9]+)'  # Get the identifier `#<id>`

Unit tests are good, but they are not somewhere I would think to go for documentation on this sort of thing. At a minimum it would be good to note that the reader should consult the unit tests, but I really think there should be detailed documentation on these. Regex get nightmarish very quickly, and a bit of documentation here would save everyone who needs to maintain these many painful minutes.

I can understand wanting to keep all the database code in one place, but perhaps its more appropriate to have a package devoted to that rather than a single, giant module.

The problem I (and I expect other new-comers) have is one of discover-ability. Even if I know pagure/lib/__init__.py is where all the database work is done, finding if what I want exists is a huge chore. If I want to work with issues, I have to search through all the functions in here to find the one I want (or to discover the function doesn't exist yet). If, on the other hand, all the functions for working with issues in the database was in a issues module inside a db package, it's a much shorter search.

I'm used to the Django world where actions on a single instance of my models is a function defined on the model definition, actions on many instances of a single model live on the query manager, and actions involving several different models are either in a controller package or directly in the Django view function.

I don't expect (or even want) to replicate Django, but having clear organization like that is, in my opinion, hugely helpful for both seasoned contributors and new contributors alike.

This is something that at first I'm not entirely sure but I could be pushed into, however I believe it is outside the scope of this PR as it would involve quite some code shuffling.

It's up to you. I tend to prefer PRs that introduce something new to make adjustments based on review in the same PR, but this one is so large I'm okay with it being a follow-up PR if you are.

Since you didn't approve that PR already and are now asleep, I'll adjust it :)

Ok I added the changes to make str(log) working and fix the unit-tests accordingly (as well as for some of the changes made before).

pep 8 crossing the 80 characters limit

s/formato/format

Pulled a patch tested it , all seems to work but I need to click on the cell to show the activity which is fine but for current day activity it shows by default . apart from that works good for me :thumbsup:

As it currently stands, I can see user activity on private issues in the report, as well as the titles of the issues. If the title contains any sensitive data, this API leaks it. It isn't just this API, though. The UI does the same thing.

I think it should only be Whether to group the commits because it gives the sense of option I doubt the use of not at the end is grammatically correct.

Why is this being removed?

Does this need to be set before any imports occur? PEP8 flakers get very upset if imports are not at the top of the module.

I've seen this a lot of places and I think once or twice I've seen a comment about RHEL6 needing this, but what exactly is using this?

Yes, as I said before that's hole in our markdown processor that needs fixing :)

This isn't something I expect to be addressed in this PR since it looks like all the tests have this path manipulation, but I assume this is so you can run the tests without having Pagure installed, right? Isn't it simpler on everyone concerned to just require that pagure is in the Python path to run the tests?

ok

Thanks :)

sure :)

I has been moved to test_pagure_flask_api_user.py since the code has been moved to pagure/api/user.py

Yes it has to be before all the imports.

It basically tells python which version of sqlalchemy to import if there are more than one on the system (which is the case for RHEL6 and maybe 7)

This is to make it easier to run the tests of this file by just doing python tests/tests....py without having to deal with PYTHONPATH & all :)

It'd be good to add some tests with invalid requests. I noticed, for example, if you provide a date like 2016-31-01 or 999999 it doesn't HTTP 400 and instead returns an empty list of activities.

I would probably say "Whether or not to group the commits"

Okay, I am satisfied now that PR#1503 has been merged.

Looks good to me now :) :thumbsup:

Thanks for the review :)