Commit graph

41 commits

Author SHA1 Message Date
Bartosz Dziewoński 82069f7db5 Add test cases for T313093
Change-Id: I045be8da8fc144846312ce84b9e79d65c3d5b2b4
2022-11-14 19:17:34 +01:00
Bartosz Dziewoński 469b7720af Place replies outside transclusions, disallow replying to transcluded comments
Bug: T313100
Change-Id: I3993c96ec8d3d0add33d779860d158327985107d
2022-11-14 19:17:18 +01:00
Bartosz Dziewoński 1d6ba985be Fix parsing of non-English titles in JS tests
Same issue as b68832ace0 in PHP.

Change-Id: Ide6ebc47929b597e6dc23df696d87bb3130ce276
2022-10-17 17:10:53 +02:00
Bartosz Dziewoński 361283a332 Ship HTML test files for JS using 'packageFiles' instead of 'templates'
We originally used 'templates' because it seemed like an obvious
choice for HTML files, and because 'packageFiles' requires extra code
to include anything that isn't a .js or .json file.

However, the templates are expected to be HTML fragments rather than
whole documents, and they are parsed in a particular way that takes a
lot of code to clean up (which we needed to do, because we use the
same test files for testing PHP code).

I tried doing it in the 'packageFiles' way, and the extra code doesn't
seem that bad in comparison after all. Moreover, the 'templates'
mechanism (when used the intended way) feels vaguely deprecated in
favor of Vue.js, and I'd rather move away from it.

This makes the tests faster too (probably mostly thanks to the removal
of the clean up code) – on my machine they go from 1800ms to 1500ms.

(Simplify linearWalk tests, as we no longer need to do weird things
with document fragments to get consistent outputs in PHP and JS.)

Change-Id: I39f9b994ce5636d70fea2e935a7c87c7d56dcb26
2022-10-12 22:45:41 +00:00
Ed Sanders 045b020ca4 modifier.test.js: Improve test descriptions
Exposes that the old parser tests are asserting 0 comments added.

The tests are passing becaause testUtils.getThreadContainer returns
the empty string for both actual and expected.

Change-Id: I263a258a8db5c35a6fb1fc5ce281f902fc543038
2022-10-11 16:57:46 +01:00
Bartosz Dziewoński 77614a2d02 tests: Fix root node / container handling
Since times immemorial, and for reasons lost to history, our test code
was adding an extra <div> wrapper before parsing the HTML used for
tests. This wasn't a problem, until now, because I want to add some
tests for T303396 that need to check that the *real* wrappers present
in some test cases are handled correctly.

Changes to test cases mostly remove a leading "0/" from serialized
ranges, corresponding to removing the extra wrapper.

Change-Id: Ia50e3590538c8cd274b02d2a937ba1a3fbb4ac89
2022-03-10 18:43:58 +01:00
Bartosz Dziewoński 0030f4cb9b Disable the biggest JS modifier test cases temporarily
Bug: T303074
Change-Id: I9bd284feb4ede27aadf99904fd230d9bfd778351
2022-03-04 20:22:11 +00:00
Bartosz Dziewoński 1d43a024f9 Handle reply/topic preview entirely server-side
We were rendering the preview in a completely different way from how
we would add the real reply, and the results would be different
sometimes, particularly for multi-line comments with messed-up markup.

Render it server-side instead, in a very similar way to real replies
(generating a DOM list node and transforming it through Parsoid),
although without the whole context of the page to improve performance.

We can remove a lot of client-side code that was used solely for this.

This will allow the preview to accurately display the signatures when
we change how they are added (T278442), without us having to implement
those changes again from scratch for the preview.

Change-Id: I53341f4d4075c25b67ec3b3032bff9b8a880dcd3
2022-02-21 17:42:28 +01:00
Bartosz Dziewoński 8e44b43df0 Split off ThreadItemSet from CommentParser
Goal:
-----
Finishing the work from Iadb7757debe000025e52770ca51ebcf24ca8ee66
by changing CommentParser::parse() to return a data object, instead of
the whole parser.

Changes:
--------
ThreadItemSet.php:
ThreadItemSet.js:
* New data class to access the results of parsing a discussion. Most
  methods and properties are moved from CommentParser with no changes.

CommentParser.php:
Parser.js:
* parse() returns a new ThreadItemSet.
* Remove methods moved to ThreadItemSet.
* Placeholder headings are generated slightly differently, as we process
  things in a different order.
* Grouping threads and computing IDs/names is no longer lazy. We always
  needed IDs/names anyway.
* computeId() explicitly uses a ThreadItemSet to check the existing IDs
  when de-duplicating.

controller.js:
* Move the code for turning some nodes annotated by CommentFormatter
  into a ThreadItemSet (previously a Parser) from controller#init to
  ThreadItemSet.static.newFromAnnotatedNodes, and rewrite it to handle
  assigning parents/replies and recalculating legacy IDs more nicely.
* mw.dt.pageThreads is now a ThreadItemSet.

Change-Id: I49bfe019aa460651447fd383f73eafa9d7180a92
2022-02-21 16:22:32 +00:00
Bartosz Dziewoński 4613ae78e7 Change CommentParser into a service
Goal:
-----
To have a method like CommentParser::parse(), which just takes a node
to parse and a title and returns plain data, so that we don't need to
keep track of the config to construct a CommentParser object (the
required config like content language is provided by services) and
we don't need to keep that object around after parsing.

Changes:
--------
CommentParser.php:
* …is now a service. Constructor only takes services as arguments.
  The node and title are passed to a new parse() method.
* parse() should return plain data, but I split this part to a separate
  patch for ease of review: I49bfe019aa460651447fd383f73eafa9d7180a92.
* CommentParser still cheats and accesses global state in a few places,
  e.g. calling Title::makeTitleSafe or CommentUtils::getTitleFromUrl,
  so we can't turn its tests into true unit tests. This work is left
  for future commits.

LanguageData.php:
* …is now a service, instead of a static class.

Parser.js:
* …is not a real service, but it's changed to behave in a similar way.
  Constructor takes only the required config as argument,
  and node and title are instead passed to a new parse() method.

CommentParserTest.php:
parser.test.js:
* Can be simplified, now that we don't need a useless node and title
  to test internal methods that don't use them.

testUtils.js:
* Can be simplified, now that we don't need to override internal
  ResourceLoader stuff just to change the parser config.

Change-Id: Iadb7757debe000025e52770ca51ebcf24ca8ee66
2022-02-19 19:51:57 +01:00
Bartosz Dziewoński bacf6a8fc6 Remove unused code in JS modifier
I don't think we're going to need this in the client again.

Change-Id: Id38405c14edfd7ac45afad8f32cad64594ae7bc0
2022-01-29 21:39:45 +00:00
Bartosz Dziewoński b7cbd714ca Add tests for bullet indentation
Bug: T259864
Change-Id: If38016564b67ee7217fe7328b40973aa244ff467
2022-01-14 00:27:04 +00:00
Bartosz Dziewoński 1ce3d7ab7e Work around Karma test runner timeouts caused by large tests
Change-Id: I8c7d32dac073e1f0510e57f26ba81ff22f005f0a
2022-01-14 00:26:37 +00:00
jenkins-bot 7f329ca9a2 Merge "Enable wikis to customize the syntax used for replies" 2022-01-12 21:32:49 +00:00
Ed Sanders 34011b7a07 Parser: Pass in title of page being parsed
Will be used to parse selflinks in the future.

Change-Id: I2bc29d1c5c69cb6309f582f162f9af7d96ce8913
2022-01-12 21:17:59 +00:00
Bartosz Dziewoński 7b1053300a Enable wikis to customize the syntax used for replies
The following values for configuration variables are supported:
$wgDiscussionToolsReplyIndentation = 'invisible'; (default)
$wgDiscussionToolsReplyIndentation = 'bullet';

Bug: T259864
Change-Id: Icefad79630adc6ed35687498614e6a03ede1451b
2022-01-12 20:54:04 +00:00
Ed Sanders 3f5756f495 List methods not covered by unit tests in TODO sections
Change-Id: Ia06c3c726b7bc9758454aa8b2f7ea34cbada4c09
2021-11-13 15:27:38 +00:00
Ed Sanders 1893405635 Code style: Move var declarations inline
Change-Id: I1686603388b050ba4ec22eff23e4806cdf262b87
2021-04-22 17:43:46 +00:00
Bartosz Dziewoński 44f2209abf Trim signatures when added in an empty existing node, too
Add unit tests for appendSignature().

Bug: T276612
Change-Id: Ic44c52f4d54492e092f9396c626380e2637b6f0f
2021-03-08 23:38:46 +00:00
Bartosz Dziewoński ccd9e411d2 Allow updating the expected results when running PHP tests
This is similar to the code we already have in JS tests, but instead
of printing to the console where you have to copy-paste from, it just
overwrites the files.

Also, update all of the expected results by this method.

Changes in the expected outputs:
* In JSON files, the "warnings" are now always in the same place
  regardless of the type of the warning.
* In all HTML files, self-closing tags now include the trailing slash,
  some characters are no longer encoded as entities when not necessary,
  and attributes may be single-quoted when that makes them shorter.
* In Parsoid HTML files, the header is no longer terribly mangled.

Other notes:
* CommentParserTest.php: Change the output of serializeComments()
  to be in similar order as in JS, to reduce the diffs in this commit
  and because it's a better order for humans.
* modifier.test.js: Remove some hacks that were working around small
  inconsistencies between the previous expected outputs and the actual
  outputs.

Change-Id: I9f764640dae823321c0ac35898fa4db03f1ca364
2020-08-04 03:05:28 +02:00
jenkins-bot 889de1bcdf Merge "Improve detecting typed signatures" 2020-07-22 01:43:40 +00:00
Bartosz Dziewoński 80e52e1155 Improve detecting typed signatures
* Remove the existing approach for detecting signatures that only
  worked in source mode; remove autoSignWikitext()

* Use the same approach for auto-signing in source mode as we have
  already used in visual

* In both modes, detect whether the user has already typed a signature
  at the end of their comment in the modifier, and if so, don't add a
  signature

* Add test cases for the detection

Bug: T255738
Change-Id: I791d3035cb1ffc33ce3966d4617a25d08700c35b
2020-07-22 00:00:53 +02:00
Ed Sanders a2431fe006 Refactor CommentParser
* Pass rootNode to the constructor
* Rename getters to match CommentItem/HeadingItem/ThreadItem
  value classes.
* Always build the thread tree so CommentItem's always have
  and ID and replies/parent.

Change-Id: I508be9534de59016ff806e3d84edcbb1c76cb0c6
2020-07-20 23:38:10 +01:00
Ed Sanders 6459e7dc82 Move wikitext modifiers to modifier.js
Re-create methods in PHP.

Change-Id: Iae6117b65e3b8f50ecc68e1e3ea17c8359bdcb06
2020-07-01 17:06:02 +01:00
Ed Sanders 62c0080850 Fix whitespace handling in unwrapList
Update 'referenceNode' before modifying the DOM.

Bug: T254308
Change-Id: I76af898e238c1e6d3db96d3e6156e80e55c87820
2020-06-03 13:54:36 +01:00
Ed Sanders 0d14fcea6a wt->visual: Don't unwrap template lists
Bug: T253150
Change-Id: I1584d9834e29c38edf4234f2f022c1c48bfd485f
2020-06-01 22:32:23 +01:00
Ed Sanders c5d1029b25 Move /cases and /data up to /tests
Theses are no longer QUnit specific.

Change-Id: I5f3cca1ff686922e0cdaaedb80858f37df04799a
2020-05-18 21:47:17 +01:00
Ed Sanders d1e58841af Rename removeListItem to removeAddedListItem and remove in PHP
This method shouldn't be required on the server. Leave comments
relating to it in addListItem so JS & PHP can be kept in sync.

Change-Id: I849fac660faf6e750272c20776f96b9250f96b1b
2020-05-18 19:25:08 +00:00
Ed Sanders 4397bda8cb Modifier: Separate out test data
Change-Id: I63d868a47daebee180c229b424311d10b80d2e89
2020-05-11 16:58:00 +01:00
Ed Sanders 56d8721fd7 Switching between VE & wikitext (plain)
Bug: T234403
Change-Id: I84a639ab8e31ec9f683a63b0ecf184f426df09dd
2020-05-06 18:03:36 +01:00
Bartosz Dziewoński 047e8eb82c Fix inserting replies inside a <pre>
In the future, we should think about a better solution that also
handles other elements (T250126), but this is an easy fix for now.

Bug: T250512
Change-Id: I1321f0da523ddb4a999b8c453b9094a267b38ae2
2020-04-27 23:23:31 +02:00
Bartosz Dziewoński 55b43a70d4 Fix skipping to the end of the paragraph when there is no wrapper
Bug: T249217
Change-Id: I77847901d98f56dbcbcf379c6c17f41e06a3a939
2020-04-03 18:11:59 +00:00
Bartosz Dziewoński 216516f719 Rename tests/qunit/utils.js to .../testUtils.js
We now also have modules/utils.js and I found the identical name confusing.

Change-Id: I6895748528241e0368a79eab9dcf3123afaf5f87
2020-03-24 22:29:48 +00:00
Bartosz Dziewoński 04365c0188 Merge RL modules which are only loaded by 'ext.discussionTools.init'
Bug: T240474
Change-Id: I1b83aa18666be8f1ea6a3602b299f92574d42cb7
2020-03-14 14:33:23 +01:00
Bartosz Dziewoński e3e4ef9de4 parser: Detect comments transcluded from another page
When trying to reply to a comment that is inside a transclusion,
detect if it's transcluded from a subpage or simply wrapped in a
template, and show appropriate error messages.

References:
* VisualEditor ve.dm.Converter#getAboutGroup()
* VisualEditor ve.dm.ModelRegistry#matchElement()
* Parsoid Linter#findEnclosingTemplateName()

Bug: T245694
Change-Id: If3dd1ebbf1d02ee4379c200019bfc3a8ec02325b
2020-03-09 20:28:56 +01:00
Bartosz Dziewoński 606d6b34ec Add reply links at the end of a line, even if the signature is in the middle
Previously they were added at the end of the text node containing the
timestamp, which was usually the end of the line, but not always.

And also fix the same problem for inserting the actual replies (or
reply widgets). This replaces an undocumented hack that prevented our
own reply links from triggering this bug (without it, the reply widget
would be inserted before the reply link rather than after).

While we're here, remove unintentional spacing that appeared before
some reply links, caused by trailing whitespace in text nodes.

Add tests for all of the above.

Bug: T245695
Change-Id: I354b63e2446bb996176a2e3d76abf944127f307e
2020-03-02 21:39:37 +01:00
jenkins-bot 0e22b4c79f Merge "Clean up the interface after discarding a reply" 2020-02-27 23:05:00 +00:00
Bartosz Dziewoński d068d2ef2c Clean up the interface after discarding a reply
Bug: T245574
Change-Id: I016a7a5c44e0d15a153143177976cceb8d6d3d1b
2020-02-27 18:09:34 +01:00
Bartosz Dziewoński 2a93ed11b4 Rearrange test cases
I think directories like this make more sense for adding more test cases.

Depends-On: I9153851fe162c012967fda00d3e1f81964a8dce9
Change-Id: Ibc72b747a75c72643c1fc04eae49bd15656e8104
2020-02-25 00:19:30 +00:00
Bartosz Dziewoński 890588f36a Pick reply insertion point based on parser tree, not DOM tree
I don't like that I had to special-case `<p>` tags (top-level
comments) in this code. I feel like it should be possible to handle
top-level comments and replies in a generic way, but I couldn't find
a way to do it that actually worked.

Notes about changes to the behavior, based on the test cases:

* Given a top-level comment A, if there was a "list gap" in the
  replies to it: previously new replies would be incorrectly added at
  the location of the gap; now they are added after the last reply.
  (T242822)

  Example: "pl", comment at "08:23, 29 wrz 2018 (CEST)"

* Given a top-level comment A and a reply to it B that skips an
  indentation level: previously new replies to A would be added with
  the same indentation level as B; now they are added with the
  indentation level of A plus one. (The old behavior wasn't a bug, and
  this is an accidental effect of other changes, but it seems okay.)

  Example: "pl", comment at "03:22, 30 wrz 2018 (CEST)"
    and reply at "09:43, 30 wrz 2018 (CEST)"

* Given a top-level comment A, a reply to it B, and a following
  top-level comment C that starts at the same indentation level as B:
  previously new replies to A would be incorrectly added in the middle
  of the comment C, due to the DOM list structure; now they are added
  before C. (T241391)

  (It seems that comment C was supposed to be a multi-line reply that
  was wrongly indented. Unfortunately we have no way to distinguish
  this case from a top-level multi-line comment that just happens to
  start with a bullet list.)

  Example: "pl", comments at "03:36, 24 paź 2018 (CEST)",
    "08:35, 24 paź 2018 (CEST)", "17:14, 24 paź 2018 (CEST)"

* In the "en" example, there are some other changes where funnily
  nested tags result in slightly different results with the new code.
  They don't look important.

* In rare cases, we must split an existing list to add a reply in the
  right place. (Basically add `</ul>` before the reply and `<ul>`
  after, but it's a bit awkward in DOM terms.)

  Example: split-list.html, comment "aaa"; also split-list2.html
    (which is the result of saving the previous reply), comment "aaa"

* The modifier can no longer generate DOM that is invalid HTML, fixing
  a FIXME in modifier.test.js (or at least, it doesn't happen in these
  test cases any more).

Bug: T241391
Bug: T242822
Change-Id: I2a70db01e9a8916c5636bc59ea8490166966d5ec
2020-01-23 21:13:12 +01:00
Bartosz Dziewoński da732843f3 Integration tests for the modifier
Document the current behavior of the modifier (which inserts the
replies into the DOM tree), so that we can more easily see the effect
of changes in I2a70db01e9a8916c5636bc59ea8490166966d5ec.

Basically, add a reply to every comment, and dump the resulting HTML,
comparing it to previously generated expected HTML (which can be
checked visually). Have a look at the new HTML files.

Notably, the very first section in the "pl" example demonstrates a
case of wrong reply location due to list gap :) (T242822).

Change-Id: I4aed0f0b112f53d98e3fe1da4d40db8687c7e537
2020-01-22 00:58:06 +01:00