More tag module bugs :(

Hi is this a good replica of the bug

Please I think a lot of bugs has been fixed in the fabricator but are still being left as bugs, cause when I replicate some of the bugs I realize they have been fixed , is this correct or are all the bugs in the phabricator still unfixed?

I can still reproduce this on latest alpha version. Here's a video:

In T353437#9648027, @Novem_Linguae wrote:

I can still reproduce this on latest alpha version. Here's a video:

By alpha version do you mean the version for MediaWiki or PageTriage, also how do I install this particular alpha version, my current version does not have the icon sidebar at the right in the video above.

By alpha version, I just mean the latest commit on master, as opposed to an official release version.

The Page Curation toolbar will display with these two things are true:

• you are logged into an account that has the "patrol" permission, for example an administrator account,
• you did not create the article yourself (a security feature that keeps people from patrolling their own articles).

So to get the toolbar to show up in a localhost, I'd do something like...

• log out
• create an article
• log in as administrator
• visit the article
• toolbar should now show up

Change #1017276 had a related patch set uploaded (by Rockingpenny4; author: Rockingpenny4):

[mediawiki/extensions/PageTriage@master] Fixes unnecessary line break on maintenance tagging

https://gerrit.wikimedia.org/r/1017276

Hey! I have made a patch for this issue and it fixes line break for single tags :
.

I also feel it would be better if we fix it for multiple tags and would like to work on it. Can you please guide me for it, once after the review for this patch.

I'd recommend fixing both in the same patch, if you're willing.

The current patch is breaking a Jest test involving {{Multiple issues}}, so we kind of have to fix it in the same patch anyway.

Although I do think we need to make one change to the failing test too. Should remove the \n before {{Multiple issues.

		return toolbar.submit().then( () => {
			expect( applyTags ).toBeCalledWith( '\n{{Multiple issues|\n{{disputed|date=today}}\n{{linkrot|date=today}}\n}}\nThis is a page.', [ 'disputed', 'linkrot' ] );
		} );

A patch like yours that just changes Wikicode is a great candidate for TDD. You could write a couple extra tests, then just tweak your code until they all pass.

Here's my notes on how to run Jest tests locally: https://en.wikipedia.org/wiki/User:Novem_Linguae/Essays/Docker_tutorial_for_Windows_(WSL)#Jest

In T353437#9648875, @Novem_Linguae wrote:

By alpha version, I just mean the latest commit on master, as opposed to an official release version.

The Page Curation toolbar will display with these two things are true:

• you are logged into an account that has the "patrol" permission, for example an administrator account,
• you did not create the article yourself (a security feature that keeps people from patrolling their own articles).

So to get the toolbar to show up in a localhost, I'd do something like...

• log out
• create an article
• log in as administrator
• visit the article
• toolbar should now show up

OK, thanks.

In T353437#9694086, @Novem_Linguae wrote:

I'd recommend fixing both in the same patch, if you're willing.

The current patch is breaking a Jest test involving {{Multiple issues}}, so we kind of have to fix it in the same patch anyway.

Although I do think we need to make one change to the failing test too. Should remove the \n before {{Multiple issues.

		return toolbar.submit().then( () => {
			expect( applyTags ).toBeCalledWith( '\n{{Multiple issues|\n{{disputed|date=today}}\n{{linkrot|date=today}}\n}}\nThis is a page.', [ 'disputed', 'linkrot' ] );
		} );

A patch like yours that just changes Wikicode is a great candidate for TDD. You could write a couple extra tests, then just tweak your code until they all pass.

Here's my notes on how to run Jest tests locally: https://en.wikipedia.org/wiki/User:Novem_Linguae/Essays/Docker_tutorial_for_Windows_(WSL)#Jest

Thanks for the review, I have fixed the {{Multiple issues}} test and will look into writing jest tests and update soon.

(Update) Modified the patch for multiple tagging as well and also tweaked the tests. Here's a video:

Should any other changes be made?

I see a bunch of bugs in your video. Such as duplicating that {{notability}} tag (should detect that it is there and not place it twice, T41319). Or not putting the existing tags in {{multiple issues}} (T361988). This whole section of the code needs a lot of work.

But those bugs are unrelated to this specific ticket and can be handled in other tickets.

Your changes look good. I gotta run, but I think this patch will be merged soon. Thanks for your work on it.

In T353437#9694492, @Novem_Linguae wrote:

I see a bunch of bugs in your video. Such as duplicating that {{notability}} tag (should detect that it is there and not place it twice, T41319). Or not putting the existing tags in {{multiple issues}} (T361988). This whole section of the code needs a lot of work.

But those bugs are unrelated to this specific ticket and can be handled in other tickets.

Your changes look good. I gotta run, but I think this patch will be merged soon. Thanks for your work on it.

Thanks for the review , I will look into the associated tickets and figure out a fix for them.

In T353437#9694492, @Novem_Linguae wrote:

I see a bunch of bugs in your video. Such as duplicating that {{notability}} tag (should detect that it is there and not place it twice, T41319). Or not putting the existing tags in {{multiple issues}} (T361988). This whole section of the code needs a lot of work.

But those bugs are unrelated to this specific ticket and can be handled in other tickets.

Your changes look good. I gotta run, but I think this patch will be merged soon. Thanks for your work on it.

Thanks for the review , I will look into the associated tickets and figure out a fix for them.
Would also appreciate review on the patch I made for T336604 as well, if time allows.

Change #1017276 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Fixes unnecessary line break on maintenance tagging

https://gerrit.wikimedia.org/r/1017276