Add analytics pageview module, spec, and tests#350
Add analytics pageview module, spec, and tests#350d00rman merged 14 commits intowikimedia:masterfrom
Conversation
|
@milimetric Hm, the PRs seem to to get duplicated (#343) Can I close the first one? |
There was a problem hiding this comment.
The source of your problem is indentation here. Should be:
x-request-handler:
- get_from_backend:
# Note how `request` is one-tab more indented than it your code.
request:
uri: ...There was a problem hiding this comment.
Done.
oops. I noticed the error was about a syntax problem, but I didn't know how to go from there to where the problem was. Do you all use a linter or just know what problems to look for?
There was a problem hiding this comment.
@milimetric The error message was indeed not too descriptive, I would've get confused if I didn't know what's going on. We should make an error message more self-explanatory
|
I think after fixing errors I've pointed the tests should pass. I'll take one more closer look after that, but in general this all looks good. The only big think left is bike shedding on spec's naming. |
|
This kind of brings up the question of whether RESTBase should eventually contain and test a union of all possible RESTBase configs. Right now we bundle the Wikimedia sub-specs & test them along with RESTBase-the-framework, but at some point we might want to separate the config from the code. |
There was a problem hiding this comment.
this should become /{module:pageviews}/insert-top/{project}/{access}/{year}/{month}/{day}/{data} and everything will go well
|
Except for two tiny nits this all looks good to me. (given that we didn't finish choosing the spec directory structure vs plain files, but that could be added/fixed separately. |
There was a problem hiding this comment.
Indentation for the whole function.
There was a problem hiding this comment.
The dot in .org should be escaped. Also, I think a cleaner way of doing the would be rp.project.replace(/\.org$/, ''), but it's up to you.
There was a problem hiding this comment.
A sanity check for values would be good for all three blocks here (as in, I can put in only digits, but that's not enough for the date to be valid).
There was a problem hiding this comment.
I was going to add a note, maybe I should have. The only way to fully validate the dates would be to include another library, because basic checking still wouldn't consider leap years, leap seconds, etc. Given that, I just do basic validation and assume a 404 is good enough for when people send invalid dates. If you'd like to see another library here, that's cool with me, I just didn't want to add to the dependency tree.
There was a problem hiding this comment.
Now that we have validateTimestamp we should make it work for this case too, i.e. supply it rp.year, rp.month and rp.day and see if the date is valid.
|
Modulo some minor comments and nits, LGTM. @wikimedia/services thoughts? |
|
Thanks Marko, I think I patched everything except the date construction On Tue, Sep 29, 2015 at 5:23 PM, Marko Obrovac notifications@github.com
|
There was a problem hiding this comment.
I'm not sure we need to provide such level of detail here. For 2014-01-49 all 3 errors would happen, while year and month are valid. I think just returning smth like 'Invalid timestamp' would be enough
|
LGTM 👍 |
There was a problem hiding this comment.
Nit: should probably be queryResponder (sorry, my OCD doesn't allow me to ignore this) :P
There was a problem hiding this comment.
:) Sorry for the pun. It's not quite a responder because it's not responding to queries, it's just fixing the response. I'll name it accordingly.
|
LGTM too 👍 |
|
@milimetric just rebase on top of master and we're GTG! |
NOTE: this commit just adds the backend necessary to serve analytics data. We will add any code needed to configure a front-end in a later pull request. Bug: T107053
* Bad parameters - throw 400 error, some tests check this
* No data found - let 404 bubble up, no tests, part of framework, but I
removed the logic that was changing 404s into 200s
Add analytics pageview module, spec, and tests
5 participants
This commit just adds the backend necessary to serve analytics
data. We will add any code needed to configure a front-end in a later
pull request.
Bug: T107053