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.

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

Add Wikiapiary Extension Badge [WikiapiaryInstalls] #6678

Merged
merged 8 commits into from
Jul 7, 2021

Conversation

SethFalco
Copy link
Contributor

I thought I could go through the backlog and pick something up.

It seems there are multiple types of extensions for MediaWiki, which can be read about here:
https://www.mediawiki.org/wiki/Manual:Extensions

Wikiapiary labels them as "Extensions" and "Skins".
Technically, this works for "Farms", "Generators", and "Hosts" as well, so it could be worth renaming this? 🤔

@shields-ci
Copy link

shields-ci commented Jun 27, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @SethFalco!

Generated by 🚫 dangerJS against 30dbe0c

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Jun 27, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 June 30, 2021 02:54 Inactive
@SethFalco SethFalco changed the title [Wikiapiary] Add Wikiapiary Extension Badge Add Wikiapiary Extension Badge [WikiapiaryExtension] Jul 1, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 1, 2021 13:58 Inactive
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

There's a couple of cases that are not properly handled here

Firstly there is no handling for the case where the extension (or whatever) does not exist e.g:

  • /wikiapiary/extensions/Extension/doesnotexist
  • /wikiapiary/extensions/notatype/doesnotexist

Secondly, there's some errors around case-sensitivity. If I call for example:

  • /wikiapiary/extensions/extension/ParserFunctions or
  • /wikiapiary/extensions/Extension/parserfunctions

then an error is thrown because the param case doesn't match the response case.

services/wikiapiary/wikiapiary-extension.service.js Outdated Show resolved Hide resolved
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 2, 2021 02:43 Inactive
@SethFalco SethFalco changed the title Add Wikiapiary Extension Badge [WikiapiaryExtension] Add Wikiapiary Extension Badge [WikiapiaryInstalls] Jul 2, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 2, 2021 02:46 Inactive
services/wikiapiary/wikiapiary-extension.service.js Outdated Show resolved Hide resolved

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

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

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

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.

Co-authored-by: chris48s <chris48s@users.noreply.github.com>
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 6, 2021 14:38 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 6, 2021 15:09 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 6, 2021 21:39 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 6, 2021 21:45 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6678 July 6, 2021 21:49 Inactive
@repo-ranger repo-ranger bot merged commit 23678fe into badges:master Jul 7, 2021
@paulmelnikow
Copy link
Member

Hey @SethFalco I'm curious what's gotten your interested in working on Shields! Would you like to have a chat over Zoom sometime? (Face-to-face is great but voice is fine too if you prefer.) No pressure! If you're interested, want to reach out to me on Discord and we can schedule something?

@SethFalco
Copy link
Contributor Author

Hey @paulmelnikow!

There's no special reason, to be honest. I just like contributing to projects I depend on or find interesting. ^-^'
I saw the backlog and figured it'd be fun to close a service-badge issue occasionally.

And sure, I'm up for a chat if you're certain! I have Zoom and Discord already, if you want you can just add me? (Seth#1276)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Badge request: MediaWiki extension usage counts from WikiApiary
6 participants