This is now possible since the render functions return jQuery objects.
All this code is exclusively used in the pagePreview.js file, and
doesn't need to make the already very big renderer.js file even
bigger.
Note the tests for all renderers have always been collected in a
single file. That's why the test case does not move.
Change-Id: I0c24638751c5f0e93d2bc0f3f4bb61fa0cf50d15
This just adds a simple test if passing the id of the clicked
reference source footnote works.
Bug: T213905
Change-Id: Ifc6549aa0203f19a5b24fa854b0aaf0cfb25674d
To make sure that we enable the link highlighting in the Cite extension we want
to trigger the click handler on the original footnote link. This is done by
passing the id of the source element to the model and the renderer.
Bug: T213905
Change-Id: I0bd59ac326269f3c0850946851fb79b611dc2a57
As discussed, the $( '<a>', { id: 'foo' } ) syntax is bogus because
plugins are able to *change* it. It's not just a list of attributes,
but whenever there is a method with the same name, that method will be
called instead. This means the result of this feature is unpredictable.
This patch also streamlines a few other jQuery calls that can be
shortened.
Bug: T214970
Change-Id: Ib58b8673c7ce41139f926c845c1b3adfbfde1b26
A lead section is essential for a summary in the /restbase/
content service. On commits we test the mediawiki endpoint but
against beta cluster we test the restbase endpoint.
I overlooked this in If855c7c0a2ad65d96d03d6a1411b453ecbe8752b
Bug: T214974
Change-Id: I9959d7ae463c4e1d4fa5345fdb59fe1b2152d49e
The previous Popups test page pointed to the "Main page" which as
of Ie15487184a7f9fc08603fc42cfad3aeac6642dcc has specially handling
This makes a new test page "Popups test page 2" that is linked to from
"Popups test page" which previously linked to the Main page which
now leads to the display of a broken Popup
This gets our test fixed but the problem with main page previews
will remain (T215080)
Bug: T214974
Change-Id: If855c7c0a2ad65d96d03d6a1411b453ecbe8752b
This solves the (I believe) only regression we introduced: A bad fake
reference like <span class="reference">[[Other article#Section]]</span>
showed a page preview for the "Other article" before we introduced
reference previews, but would have shown nothing after I9ec57e0.
Checking if the link is a self-link solves this and possibly more related
issues. Only self-links can point to a footnote on the same page.
Manually created fake-references like
<span class="reference">[[#Section]]</span> still have a chance to show
nothing in case the manually created HTML does not strictly follow the
expectations in the gateway. There is not much we can do about this. We
should not accept any arbitrary HTML but need to make at least *some*
assumptions.
Bug: T214970
Change-Id: I86e91bf45c3ae4c6a4086f7f1c7b1280fd400d17
I do find these very confusing and would like to remove them:
* The test setup looks like these popup types are going to use
these properties. But they don't. They are not even trying to
access these properties.
* There are no assertions that make sure these properties are
*not* used. It would be possible to add something like this,
but I honestly think this is not worth it.
We might need to reflect this in the PreviewModel documentation
in src/preview/model.js. I would like to do this in a separate
patch.
Bug: T214970
Change-Id: I136112bfea7f732d2673bcb8c69aba9defe6ba85
This tests the newly introduced code that decides if page or reference
previews should be used in the handling of a dwel event.
Bug: T214971
Change-Id: Ib20d00b7b9ee9b1ed82763137ec62e468e8f05f9
This is as preparation to introduce a gateway switch that decides if the
page or reference gatway should be selected. Moving that code to it's own
realm makes that path better testable.
Bug: T214971
Change-Id: I5efa9fb8f63f1487c627eb9a3f1fe47f43c611cc
This installs a series of safety nets:
* The selector [href*="#"] skips links without a fragment.
* It's still possible that a fragment exists, but is empty.
mwTitle.getFragment() checks this.
* The gateway does not assume the element exists, but checks this first.
If there is no such element, the gateway aborts the request in a way
that no error popup is shown. This is currently only possible with the
`{ textStatus: 'abort', xhr: { readyState: 0 } }` response as seen in
this patch. We might need to introduce a new, more clean way to silently
quit a fetchPreviewForTitle() call.
* The test for the reference gateway finally covers the scraping code.
Bug: T214970
Bug: T214971
Change-Id: I9ec57e0fbb0d21beaaa7b359c1c2bef64d2c14f5
Including tests for all situations.
I believe it is impossible or extremely hard to actually abuse any of
these places. All these data are not extracted from the current page, but
delivered either by MediaWiki's api.php or a RESTful endpoint, as
configured via $wgPopupsGateway and $wgPopupsRestGatewayEndpoint. A
possible attacker would need to write it's own endpoint (which must either
run on the same server or somehow ignore the CSRF token), and set the
value of mw.config.values.wgPopupsRestGatewayEndpoint on the client to
this endpoint – which requires just *another* attack vector to be able to
do this.
It's "the right thing"(tm) to escape all this anyway.
I found two possibly relevant security reviews of this extension, T88171
and T129177, resolved in 2015 and 2016.
Bug: T88171
Bug: T129177
Bug: T214754
Bug: T214971
Change-Id: I1d118c9ccaea434a253a772d18139b9b077118ab
Instead of maintaining a list of named constants (which must be updated
every time we want to add a new test with a new message), the mock now
behaves like MediaWiki's build-in qqx dummy language code and returns
the message key in brackets. The additional benefit of using the
HTML-like <…> characters is that this will automatically test if the
messages are properly HTML escaped.
Bug: T214970
Change-Id: Id7911036a7b582aff21acf911a826b5421a55938
This will affect all links, including [[Other page#Fragment]] for
example. But it will not have much of an effect there. The mw.Title
class is able to understand strings like "Other page#Fragment". All
old code calls title.getPrefixedDb() on the result. This will *not*
include the fragment. Only the new code will use title.getFragment().
I made sure this does not affect regular page previews, even when the
link is something like [[Other page#Fragment]].
Bug: T213415
Change-Id: I15611a44aa0477cc5e48ee4b12aae3cd981d977c
Mocha discourages the usage of arrow functiones in the test specs since
Mocha context can't be accessed from inside.
I stumbled across this when using this.skip(); in the reference preview
selenium tests. Since it took me some time to figure out why it was not
working, I guess it's better to avoid lambdas generally there.
See https://mochajs.org/#arrow-functions
Change-Id: I95cb183ac88e9a624c449a8f9addbe84bf76c335
I guess both is fine: either having the default in the gateway (as it
was before), or in the renderer (as this patch proposes). I, personally,
feel better with having it closer to where it is needed. This way it's
not possible to accidentially deliver a model object with an empty title.
The renderer will catch this.
At the moment we don't know exactly how we will fetch other titles (e.g.
"Book").
This change is split from I15611a4 where it was a little misplaced.
It also includes a test for the default fallback title.
Bug: T213907
Change-Id: I8ec3ddc21a417da7f95feff7b080cbd60d5472e7
Including tests. I also changed the title to include quotes as well,
even if not critical in that case.
Bug: T214754
Change-Id: I2f92a5714f7adc229a003f9167bcc9afdbc55583
This is documented at http://usejsdoc.org/tags-type.html, but not in many
other places, especially not in the JSDuck documentation.
The {!…} syntax means "can not be null". This is the default anyway.
The {?…} syntax means nullable. In a few situation is was used when a
parameter can be undefined. I decided to remove it everywhere and replace
it with {…|null} when appropriate, because this is much more explicit. Less
syntax to remember.
Note I'm intentionally not using the […] syntax when a parameter is followed
by non-optional parameters. Actually skipping a parameter in such a situation
would mess the parameter order up. Having optional parameters not at the end
is sometimes used as a feature in JavaScript code, but not in this codebase,
as far as I can see.
Change-Id: Ie370cfe08c32d1af5b0341951bed044fc3511c57
I finally found the issue. It was an incomplete mock for the
mw.html.escape() function that would return the string unescaped.
Bug: T213415
Bug: T213908
Change-Id: I198393b3c72771e4018f79913ddb9f4cb2c0d4de
Excluding tests for the renderer which keeps failing. This will be
readded in a later patch.
Bug: T213415
Bug: T213908
Change-Id: If79fa3d0a7a20f121b1ceda6e0e33ad691b1ad30
This does make generic `array` type hints more specific when possible.
I'm also applying my personal best practice to not have any @return
documentation on test @dataProviders. These don't provide any useful
information, and can't. The best type we could use is `@return array[]`,
but that would be the same for every single data provider. Copy pasting
these comments around is of no real value.
Also it was already inconsistent as some did not had this comment.
Change-Id: Id401c7e32493b6a9faaf6d47cddc01e2227102af
The test setup was slightly refactored to be more general about
the type of the popup.
The additional reference links on the test page were added mostly
to be prepared for further tests of more complex cases.
On the CI the tests should be executed with having reference
popups enabled. The code tries to skip test when the feature is
disabled.
See I17687c62cc8d738a4eb41738c9ce6662a5ec68d8
and I1eb7409aa3bd111c2e461dfe245d95f7e78d416c
Bug: T213415
Change-Id: I74110c6227596ff10c75f5f0b0da3d952f11a239
It's not exclusively about page summaries any more.
We had a few suggestions in mind:
* get, fetch, request, or issueRequest. But I feel these are all to
generic and don't describe well what the method does. As a reminder:
It expects a Title object and returns a promise, which returns a
PreviewModel object, which contains an HTML "extract".
* fetchPreview? I feel this can still mean to many things.
* fetchPreviewModel? But we don't really need to repeat that it will
return a model object.
So I went for fetchPreviewForTitle. What do you think?
Bug: T213415
Change-Id: Icb32c63cec82f72453dc1507c9f8b8d461fd4f4c
This is a prerequisit for the later patch Ie0ccb03.
Any JavaScript code can check this feature flag via
mw.config.get( 'wgPopupsReferencePreviews' )
Bug: T213415
Change-Id: I17687c62cc8d738a4eb41738c9ce6662a5ec68d8
This is split from the current draft patch Ie0ccb03. This is part of a
series of very small patches that prepare the code for new types of popups.
We decided to not add code for other types of popups to the existing
createGateway() function, but introduce new files and functions instead.
Renaming, for example, the existing `gateway` variable name will make it
much more obvious which of the future gateways does what.
Bug: T213415
Change-Id: Ifcbc3ba53d0ab9ef67adf1f314defc76b4f89e89
This is split from the current draft patch Ie0ccb03. This is part of a
series of very small patches that prepare the code for new types of popups.
Bug: T213415
Change-Id: I00d46a716c0e6ada82ffc0034a7dd5582363c657
The code literally explains itself. The comments don't add anything
to this. They are more distracting because one must read them first
to understand they don't contain anything.
Change-Id: I6f152962ec634ae15d2bff4472e332453cb9b0bf
As far as I can see this is not an integration test, because linkTitle.js
does not interact with the document (in contrast to footerLink.js, which
does).
Change-Id: I8b611263020fe597efb63d8a0080b996ffc7dde4
I figured a single "@covers ::__construct" in PopupsContextTest should
be enough, especially since this constructor is super trivial.
There is nothing to test in NullLogger. I also removed the duplicated
documentation as it does not really apply, and is still available in the
base class.
Change-Id: I9a2e4b06e5bfd015efa4a92ba802c942290ec49d
Replace $.noop dependency with inline empty function.
The tests relied on a single function instance to pass. A toString()
comparison of the noop function cannot be used as they differ when
coverage is enabled.
Change-Id: I641801593beb240a8f7d06e388a0e41dc8a25bc6
Some comments are copy-pasted from other, unrelated code. Documenting
a PHPUnit test file as being a "module test" is, I would argue, somewhat
pointless.
Change-Id: Iac28dc9c7b3e321b682e94c6a48efb2db41ca5f7
According to https://tools.wmflabs.org/coverme/?repo=Extension%3APopups
these might be relevant to cover, and already are, but the required
@covers tags have been forgotten.
Note that the MWEventLogger class does not have a dedicated test. This
is fine because it is exclusively used by the factory (as it should),
can be considered part of it, and in this case it's fine if one test
covers both.
However, none of the log() methods is covered by any test. This is for
a later patch.
Change-Id: Ic1391f7a921d76796c4648ba59df64e793c8feae
The following upgrade was made:
eslint-config-wikimedia | 0.8.1 | 0.9.0
The upgrade of eslint-config-wikimedia removes the need of
eslint-plugin-qunit as a peerDependency because it is now a hard
dependency [1] so it was removed in our package.json.
It appears the biggest change in the upgrade was the use of separate
profiles [2]. Given this, our root .eslintrc.json was updated to extend
from 'wikimedia/client'.
Several test files were flagged by the upgraded linter and were fixed in
this patch. Additionally, our build file was flagged for having too many
statements on one line. This rule was turned off in .eslintrc.es5.json
[1] https://github.com/wikimedia/eslint-config-wikimedia/blob/master/package.json#L48
[2] https://github.com/wikimedia/eslint-config-wikimedia/blob/master/CHANGELOG.md#090--2018-11-19
Bug: T209314
Change-Id: I29db72e77f04a327bc9c2b558c6d53849287bb80
