Hi, @Hridyesh_Gupta I will remove you from "assigned to" as I will plan to submit Cat-a-lot project under Outreachy as there is no WMF GSoC in 2025.

Also, please notify me if you were already working on this and plan to submit a fix. In that case, I will remove it from the Outreachy sample tasks and assign it back to you.

Hey @Zache
I'd like to start working on this task. Please let me know if someone else is already working on it. If yes, then would it still be okay for me to contribute?

We all only know what's written in this very task, I'm afraid :)

In the provided example:

• The old revision had the category: United States Office of Management and Budget
• The new revision changed this to United States Office of Management and Budget officials

How does this show self categorization?

The category which is edited is Category:United States Office of Management and Budget officials ie. it same than the category name which was added.

Btw. To be able to select categories it needs to be enabled from cat-a-lot's preferences. (ie. the menu which didn't work in Beta Commons, you need to check the menu on the real commons)

Thanks for the explanation! I was able to successfully replicate the bug. Here’s what I did:

• Went to a page in Commons.
• Enabled the "Allow categorizing pages (including categories) that are not files" option in Cat-a-lot preferences.
• Selected a category and added it to itself.
• Cat-a-lot allowed the change, even though this should not be possible.

Expected behavior: Cat-a-lot should prevent this edit.
Actual behavior: The edit is allowed.

Now that I can replicate the issue, I will start working on a fix.

I’ve implemented a fix that correctly prevents self-categorization in Cat-a-lot. Currently, if the script detects any self-categorization, it blocks the entire operation (i.e., none of the selected categories are copied or moved).

However, I wanted to confirm the expected behavior:

• Should the script completely stop the action if self-categorization is detected? (Current behavior)
• Or should it only prevent the problematic category from being copied/moved, while allowing other valid changes to proceed?

If the latter is preferred, I can modify the logic to filter out only the invalid category while executing the rest. Please let me know your thoughts!

I think that only prevent problematic category from being copied/moved, while allowing other valid changes to proceed.

If it is easy to implement it could also pause until user confirms to continue, but i think that this could be hard to implement with current LibAPI implementation and control is either run OR break without ability to pause temporarily.

Got it! I'll work on implementing that.

I’ve completed the changes to prevent self-categorization in Cat-a-lot. The script now skips self-referencing categories while allowing other valid changes to proceed. I have attached a screen recording to show the change in action. Code diff an be found here.

1. Added a self-categorization check to skip invalid operations and display a warning message.
2. Adjusted the counters to ensure the progress dialog updates correctly.
3. Ensured the progress dialog closes when all valid changes are complete.

Let me know if there’s anything else I need to address. If not, then please let me know how and where am I supposed to upload this change.

The screenshot is very helpful! I disagree with @Zache that other actions should proceed, as the self-categorization may indicate a larger user error (e.g. wrong target category was selected), but it shouldn’t be hard to support both ways: instead of an alert(), call confirm('Skipping self-categorization: '$1' will not be copied/moved. Proceed with other pages?'), and depending on the return value, either abort the whole operation (false) or just skip the self-categorized category (true).

I haven’t looked at the code very closely, but I did notice that you used a hardcoded English message. Please use the msg() function defined at the top of the file to create a translatable experience.

You also reformatted some existing code, which makes code review more difficult. Why did you do this reformatting? If you have a good reason, then it’s probably fine (just please explain), but if you had no reason, please revert those changes to make the diff smaller.

Hey! Thank you for your feedback! I’ll make the following changes based on your suggestions:

1. Confirm Dialog for Self-Categorization:
   • I’ll replace the alert() with a confirm() dialog to let the user decide whether to abort the entire operation or skip the self-categorizing category.
   • I’ll also add a new translatable message using the msg() function to ensure the message is localized.

2. Reverting Unnecessary Code Reformatting:
   • I initially reformatted some of the code to better understand it, but I forgot to revert those changes afterward. I’ll now revert any unintentional reformatting to keep the diff smaller and easier to review. Only the necessary changes will be included.

I’ve implemented the changes based on your feedback. Here’s what I’ve done:

1. Confirm Dialog for Self-Categorization:
   • I replaced the alert() with a confirm() dialog to let the user decide whether to abort the entire operation or skip the self-categorizing category.
   • I added a new translatable message (skip-self-cat-confirm) using the msg() function to ensure the message is localized.
   • To handle the HTML-encoded characters (e.g., '), I used mw.message() directly for the confirm() dialog and called .plain() to ensure the message is displayed as plain text.

2. Reverting Unnecessary Code Reformatting. Cleaner diff can be viewed here

Let me know if there’s anything else I need to address!

Hi, thanks. I like the idea of checking for category name collisions before the code initiates asynchronous API calls, because this way the check is done before any edits are made. Some notes

changes in the doSomething() function

• I would use "*pages" instead of "selection" in the variable names for consistency, as the code already uses the term "pages" in other places.
• It would be cleaner if the check were its own function, which could then be called from doSomething(). ie. like this

filteredPages = filterOutSelfCategorizations(pages, mode)
if (filteredPages.length === 0) { 
  alert(msg('no-valid-cats-after-filter'));
  this.hideProgress(); // Ensure loading message is removed
  return; 
}
...
this.counterNeeded = filteredPages.length
...

Filtering logic
It currently filters out also the category removals ( mode = 'remove' ) and most likely we would like keep these as useful.

Using alert() and confirm()
I am totally happy with using oldschool alert and confirm boxes, but if somebody would like to use dialog(), then in T386779 there will be a good example for using it so that it will support the dark mode. The alert and confirm dialogs will support dark mode natively, as they are browser components.

new getSelectedCategory() function
This is not used anywhere so it can be removed from change?

displayResults() function
Regarding the addition of hideProgress(); at the end of displayResults(): ( line 1100 ) I think it's generally better for the code to fail visibly rather than silently. If it's known to be possible for results to display while the progress dialog remains visible, that indicates a bug. In that case, the better solution would be to file a bug ticket in Phabricator and address the underlying cause.

General comment

An alternative location for the filtering logic could be within the checkTargetCat() function. There, the code would handle retrieving the normalized category name from the API. Currently, checkTargetCat() only alerts the user about disambiguation pages and redirects but doesn't stop the editing process, so its functionality would need minor adjustments to allow skipping pages. However, the downside of using checkTargetCat() is that the editing the pages would have already begun at this point. Personally, I prefer performing filtering before the editing starts.

Hey, thank you for your detailed feedback! I’ve implemented the following changes:

1. Renamed Variables: Changed validSelections to filteredPages for consistency.
2. Extracted Filtering Logic: Moved the filtering logic into a separate filterOutSelfCategorizations() function.
3. Skipped Filtering for Removals: Modified the filtering logic to skip self-categorization checks for removals.
4. Removed Unused Function: Deleted the unused getSelectedCategory() function.
5. Removed hideProgress() Call: Removed the hideProgress() call from displayResults() as suggested.
6. I’ve decided to stick with alert() and confirm() for now to keep the code simple and consistent while focusing on implementing the logic for self-categorization. Once the core functionality is stable, I’ll look into implementing custom dialogs using the example in T386779 to provide a more polished and theme-aware user experience.

Let me know if there’s anything else I need to address!

For reference: diff

Thanks, I just tested the latest version.

Messagebox text
For the text: "Skipping self-categorization: \'$1\' will not be copied/moved. Proceed with other pages?"

Cleare text could: "Skipping self-categorization: \'$1\' will not be copied or moved to \'$1\'. Proceed with other pages?

Testing procedure
It is very good that you have clear testing procedure described in edit request. However, I think that it could be even more clear if there is directly said example directory names and what to select. (example below)

1. Open category:Cat-a-lot
2. Enable Cat-a-lot UI from bottom-right corner
3. Select from Cat-a-lot preferences: Allow categorising pages (including categories) that are not files
4. Select following categories: Cat-a-lot screenshots, Cat-a-lot test images, Protected Cat-a-lot test images
5. Attempt to copy/move to another category
6. ...

Diff
You could also update the diff link in the mediawiki:gadget-Cat-a-lot.js edit request discussion so that there would not be unnecessary white space changes, as they will make it much harder to see what has actually been changed if a user is using MediaWiki's internal diff.

Okay! Thanks a lot for the feedback. I'll make the changes you suggested.

Thanks for using confirm()! I tested it locally, and I noticed two things:

• At first, it didn’t report self-categorization at all. After looking into this, it turns out you remove the first Category: from the selected category name – however, there was no Category: in the selected category name, because my local wiki is set to Hungarian, and as such, the namespace prefix is Kategória:, not Category:. To fix this, please use the normalization MediaWiki provides (which avoids most problems: which parts of the title are case-sensitive – which depends on wiki configuration –, spaces vs underscores, leading/trailing whitespace, namespace aliases – which was my motivation –; although wikis with language conversion may still experience issues) and replace
  • selectedcat.replace("Category:", "").trim().toLowerCase() with new mw.Title( selectedcat ).toString() and
  • targetcat.trim().toLowerCase() with mw.Title.makeTitle( 14, targetcat ).toString().
• If I decide to not proceed, or if there was only one category to move (which was the self-categorization), I get an alert() after the confirm(). This is unnecessary, please instead
  • if there are no non-self-referencing actions, only present the alert(), as the user has no real choice (preceding with the remaining zero pages and skipping the remaining zero pages is the same), and
  • if there are some non-self-referencing actions, but the user chose not to proceed, don’t display a second message.

Also, there are a lot of whitespace changes and deviations from the existing coding style. As far as I see, Cat-a-lot tends to follow the MediaWiki coding conventions for JavaScript code, please adhere to them (especially the whitespace parts) in new code, and avoid changing lines just for the sake of changing indentation, as they make the diff a lot larger than it should be (and the MediaWiki diff view is particularly bad at making sense out of it, I had to resort to the wikEdDiff user script to understand what you changed).

In T386783#10681588, @Zache wrote:

Messagebox text
For the text: "Skipping self-categorization: \'$1\' will not be copied/moved. Proceed with other pages?"

Cleare text could: "Skipping self-categorization: \'$1\' will not be copied or moved to \'$1\'. Proceed with other pages?

I don’t think it makes sense to repeat the category name twice. I’d rather write Skipping self-categorization: "$1" will not be copied or moved to itself. Proceed with other pages? (Also note the double quotes: the MediaWiki user interface tends to prefer double quotes over single quotes, so I’d align with that.)

In T386783#10689334, @Tacsipacsi wrote:

I don’t think it makes sense to repeat the category name twice. I’d rather write //Skipping self-categorization: "$1" will not be copied or moved to itself. Proceed with other pages?

Yes, this is better wording

Hey, Thank you for your thorough review! I’ve implemented the following changes based on your feedback:

• Replaced manual string replacement with mw.Title() normalization.
• Dialog Flow Improvements
  • Alert() if all categories are self-referencing.
  • Confirm() if mixed (skips redundant alerts when canceled).
• Message text updated to: 'Skipping self-categorization: "$1" will not be copied or moved to itself. Proceed with other pages?' . Switched to double quotes per MediaWiki conventions.
• Minimized whitespace changes to reduce diff noise.

Improved diff can be viewed here

Thanks, both bugs are indeed fixed, and the changes are also a lot easier to follow. A few notes:

• While there are no unnecessary whitespace changes anymore (thanks!), the indentation of newly introduced code is inconsistent with that of existing code. Could you please use tabs instead of four spaces, and add the missing one level of indentation?
• The if ( filteredPages.length === 0 ) at line 1135 should not be necessary: if there are some pages (checked at line 1112), and none of them are self-links (line 1121) or there are non-self-links in addition to the self-links (line 1122), there must be some links to proceed with. (Also, you try to close the progress popup before it would’ve been opened.)

Hi @Tacsipacsi, thanks for the feedback! I’ve addressed both points:

• Updated the indentation to use tabs instead of spaces and ensured consistency with the existing code.
• Removed the unnecessary if ( filteredPages.length === 0 ) check at line 1135, as you pointed out.

Let me know if there’s anything else that needs adjustment. Thanks again for your review!

Modified diff

• I still see a bunch of space indentations (to be exact, mixed tabs-and-spaces indentations).
• The JSDoc of filterPages() lacks a level of indentation. This also happens in existing code, but not consistently, and it hurts readability IMO, so I’d indent it conventionally (one tab + one space except for the line containing the /**, which has no space).
• You no longer call hideProgress(), but it’s still there. Please remove it, since it’s useless.

Hey, thanks for the feedback! I've:

• Fixed indentation – Now uses tabs consistently (no mixed spaces), similar to existing code
• Corrected JSDoc formatting – indented to align with the function for better readability
• Removed hideProgress() – Eliminated unused code

Let me know if anything else needs adjustment!

improved diff

Thanks! Mostly looks good, but the JSDoc is conventionally formatted like

	/**
	 * Filters out self-categorizing pages and returns both filtered and self-categorizing pages
	 * @param pages Array of selected pages
	 * @param targetcat Target category name
	 * @param mode Operation mode ('add', 'remove', etc.)
	 * @return Object with filteredPages and selfCatPages arrays
	 */

and not

	/**
	* Filters out self-categorizing pages and returns both filtered and self-categorizing pages
	* @param pages Array of selected pages
	* @param targetcat Target category name
	* @param mode Operation mode ('add', 'remove', etc.)
	* @return Object with filteredPages and selfCatPages arrays
	*/

so that the asterisks nicely line up.

I also realized that

		var result = this.filterPages(pages, targetcat, mode);
		var filteredPages = result.filteredPages;
		var selfCatPages = result.selfCatPages;

could be simplified to

		var { filteredPages, selfCatPages } = this.filterPages( pages, targetcat, mode );

but in this latter case, the variant you chose is just as good as my proposal, so I leave it up to you whether you use the traditional syntax or the shorter destructuring syntax.

Thanks for the feedback!

Good catch on the JSDoc formatting—I’ll update the asterisks so they line up properly for consistency.
I also appreciate the destructuring suggestion. Since both approaches are valid, I’ll leave it as-is for now, but I’ll definitely keep that pattern in mind going forward.

As far as the core functionality goes, I believe it's all done at this point? Let me know if you spot anything I might’ve missed or if there's anything else you'd like adjusted!

diff

Just FYI, I added to the ticket description a guide on howto test the change and link to the latest diff. These were for merging to the official version of the cat-a-lot.

Removing unrelated project tags