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

Add Wikiapiary Extension Badge [WikiapiaryInstalls] #6678

Merged
merged 8 commits into from
Jul 7, 2021
Merged

Add Wikiapiary Extension Badge [WikiapiaryInstalls] #6678

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3529906
feat: add wikiapiary extension badge
SethFalco Jun 27, 2021
af2ef56
fix: refactor wikiapiary badge
SethFalco Jul 2, 2021
d3de7ec
fix: display correct message when not found
SethFalco Jul 6, 2021
84f3888
fix: weird behavior with casing
SethFalco Jul 6, 2021
30ba489
fix: test malformed api response
SethFalco Jul 6, 2021
8df3ed1
chore: use options.qs for query parameters
SethFalco Jul 6, 2021
eb40619
chore: rename file to match class name
SethFalco Jul 6, 2021
30dbe0c
Merge branch 'master' into close-146
repo-ranger[bot] Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions services/wikiapiary/wikiapiary-extension.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
'use strict'

const Joi = require('joi')
const { metric } = require('../text-formatters')
const { BaseJsonService, NotFound } = require('..')

const schema = Joi.object({
query: Joi.object({
results: Joi.alternatives([
Joi.object()
.required()
.pattern(/^\w+:.+$/, {
printouts: Joi.object({
'Has website count': Joi.array()
.required()
.items(Joi.number().required()),
}).required(),
}),
Joi.array().required(),
]).required(),
}).required(),
}).required()

/**
* This badge displays the total installations of a MediaWiki extensions, skins,
* etc via Wikiapiary.
*
* {@link https://www.mediawiki.org/wiki/Manual:Extensions MediaWiki Extensions Manual}
*/
module.exports = class WikiapiaryInstalls extends BaseJsonService {
static category = 'downloads'
static route = {
base: 'wikiapiary',
pattern: ':variant(Extension|Skin|Farm|Generator|Host)/installs/:name',
}

static examples = [
{
title: 'Wikiapiary installs',
namedParams: { variant: 'Extension', name: 'ParserFunctions' },
staticPreview: this.render({ usage: 11170 }),
keywords: ['mediawiki'],
},
]

static defaultBadgeData = { label: 'installs', color: 'informational' }

static render({ usage }) {
return { message: metric(usage) }
}

static validate({ results }) {
if (Array.isArray(results))
throw new NotFound({ prettyMessage: 'does not exist' })
SethFalco marked this conversation as resolved.
Show resolved Hide resolved
}

async fetch({ variant, name }) {
return this._requestJson({
schema,
url: `https://wikiapiary.com/w/api.php?action=ask&query=[[${variant}:${name}]]|?Has_website_count&format=json`,
})
}

async handle({ variant, name }) {
const response = await this.fetch({ variant, name })
const { results } = response.query

this.constructor.validate({ results })

const [usage] = results[`${variant}:${name}`].printouts['Has website count']
Copy link
Member

Choose a reason for hiding this comment

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

We've still got a case-sensitivity issue here. e.g:

wikiapiary/Extension/installs/parserFunctions

It seems fundamentally the difficulty stems from the fact the query is case-insensitive (kind of) but the response isn't.

Would it work if we lowercase the args and lowercase the keys in the results array

results = Object.keys(results).reduce((accum, k) => (accum[k.toLowerCase()] = results[k], accum), {})

to do a case-insensitive comparison?

That would also conveniently allow us to make the options for the :variant param lowercase. In general, we tend to avoid having uppercase characters in the bit of the URL that is not the repo/package/whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh strange. 🤔
I'll update this PR later.

I tested case-sensitivity and it worked when I tried it, but I used Parserfunctions, which returned no result. (So I assumed the extension name was itself case-sensitive.)

Not sure why parserFunctions works, but Parserfunctions returns no result. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

So there is also an issue that the upstream API behaves somewhat esoterically.

Note that

wikiapiary/Extension/installs/parserFunctions causes an error (even though the API does return the stats) but wikiapiary/Extension/installs/pArserfunctions is just a not found (which obviously we can't do anything about)

Copy link
Contributor Author

@SethFalco SethFalco Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hopefully, this should be resolved now.

I've addressed the issue with case-sensitivity, using an approach similar to what you suggested. (not identical)

I've also added documentation to the badge to clarify the odd behavior.
Unfortunately, I couldn't see documentation upstream to definitively state exactly what is accepted and what isn't.

I just tested a bunch in Insomnia, and attempted to infer a conclusion from there.
Let me know if that's not favorable!

Screenshot:
image

Alternatives

One alternative approach to selecting the correct value could be to just not use a key at all.
It appears (but I do not definitively know) that we could just get an array of values from the results (as in ignore the keys) and just select the first (and only from what I can tell) property.

I just don't know if that's a good idea, as I'm not aware of the upstream behavior. 🤔

One thing you could drop an opinion, on line 97 I throw an error if the key we're after can't be found.
I don't know of a way to actually trigger than error because logically, it should never get there.

Is that fine do you think, or shall I use a different error type/message?

Copy link
Member

Choose a reason for hiding this comment

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

I just don't know if that's a good idea, as I'm not aware of the upstream behaviour.

No - me either. I think the implementation as it stands is fine 👍

One thing you could drop an opinion, on line 97 I throw an error if the key we're after can't be found.

The check should probably be if (resultKey === undefined)... rather than if (!resultKey)...
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find#return_value

I don't know of a way to actually trigger than error because logically, it should never get there.

I'm not exactly sure what you mean here. I think throwing a NotFound is probably an OK choice of exception there. If you're not sure how to write a test for it, you could write a mocked test (see the docs: https://github.com/badges/shields/blob/master/doc/service-tests.md#5-mocking-responses ) and then you can make the request return an intentionally malformed object under to test to hit that case. If you mean something else maybve you can give an example?

Copy link
Contributor Author

@SethFalco SethFalco Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not exactly sure what you mean here.

I just meant, it appears that the upstream API would never normally return a response that would trigger such an error.
In other words, I was clarifying that I was just writing defensive code, rather than covering an actual edge-case.

Since you mention it, I've tried adding a test case anyway, but it's not going well. 🤔

t.create('Malformed API Response')
  .get('/extension/installs/ParserFunctions.json')
  .intercept(nock =>
    nock('https://wikiapiary.com')
      .get('/w/api.php?action=ask&query=[[extension:ParserFunctions]]|?Has_website_count&format=json')
      .reply(200, { query: { results: { "Extension:Malformed": { printouts: { "Has website count": [ 0 ] } } } } })
  )
  .expectBadge({ label: 'installs', message: 'not found' })

I've spent a fair bit of time on this, but I fail to see what's wrong. 🤔

AssertionError [ERR_ASSERTION]: Mocks not yet satisfied:
GET https://wikiapiary.com:443/w/api.php

I can make this work if I use regular expression, but not with a string literal.
Both of the following work:

  • /.+/
  • /^\/w\/api\.php\?action=ask&query=\[\[extension:ParserFunctions\]\]%7C\?Has_website_count&format=json$/

Trial and error hell, but I can use a strict regular expression if I replace | with the URL encoded equivalent.
However, the string literal equivalent still doesn't pass.

Just wanted to share here. I'm not sure if I'm being stupid, or if it's an issue in nock, but something seems wrong here to me.

Copy link
Member

Choose a reason for hiding this comment

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

Hi Seth! 👋🏼

I would programmatically URL-encode the weird bit of your query string:

.get(`/w/api.php?action=ask&query=${encodeURIComponent('[[extension:ParserFunctions]]|?Has_website_count')}&format=json`)

Alternatively you can use .query():

.get('/w/api.php')
.query({ action: 'ask', query: '...', format: 'json' })

If that doesn't work, you can debug the nock match with all sorts of verbose detail by setting DEBUG=nock.* when you run the test. (Add .only() to the test if necessary.)

Copy link
Contributor Author

@SethFalco SethFalco Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If that doesn't work, you can debug the nock match with all sorts of verbose detail by setting DEBUG=nock.* when you run the test. (Add .only() to the test if necessary.)

Ahh! Thanks for that!
I had already tried URL encoding the parameters before.
Unfortunately, the .query method didn't help either.

However... those were my fault.
Thanks to your tip for debugging the nock requests, I found the issue was with the request being made in the service class. The request wasn't being URL encoded in the service, so the interceptor wrongly interpreted/compared it from there.

Actual

{ action: 'ask', query: '[[extension:ParserFunctions]]|' }

Expected

object {
  action: 'ask',
  query: '[[extension:ParserFunctions]]|?Has_website_count',
  format: 'json'
}

I've switched both to just use the dedicated options.qs/query property now.
I'll keep that in mind for future reference as well.

return this.constructor.render({ usage })
}
}
20 changes: 20 additions & 0 deletions services/wikiapiary/wikiapiary-extension.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict'

const t = (module.exports = require('../tester').createServiceTester())
const { isMetric } = require('../test-validators')

t.create('Extension')
.get('/Extension/installs/ParserFunctions.json')
.expectBadge({ label: 'installs', message: isMetric })

t.create('Skins')
.get('/Skin/installs/Vector.json')
.expectBadge({ label: 'installs', message: isMetric })

t.create('Extension Not Exist')
.get('/Extension/installs/FakeExtensionThatDoesNotExist.json')
.expectBadge({ label: 'installs', message: 'does not exist' })

t.create('Name Lowercase')
.get('/Extension/installs/parserfunctions.json')
.expectBadge({ label: 'installs', message: 'does not exist' })