Recursively identify renamed username of submitter #291
Conversation
|
I wish we had reviewerbot here :) |
sohomdatta1
force-pushed
the
renamed-user
branch
from
89dc710 to
ec737ef
Compare
sohomdatta1
force-pushed
the
renamed-user
branch
from
ec737ef to
d8c87f8
Compare
| this.params.u = newName; | ||
| this.getSubmitter().done( function ( user ) { | ||
| deferred.resolve( user ); | ||
| } ); |
There was a problem hiding this comment.
What if it fails? The deferred is not being rejected in that case which leaves it hanging in a never-resolved/never-rejected state.
It would be good to convert the method to return the deferreds/promises rather than manually resolve/reject in a then block. Fine to leave for later as it's problem common to the entire codebase.
|
This bug was reported again today. Looks like this patch needs a round of code review feedback to be implemented in order to move forward. |
|
The inline comments can be easily resolved, but I'm not sure of the approach. Instead of tracking down renames from log events, I think it would be faster and easier to just follow the redirect (redirect=1 API flag) while editing the talk page, as the user talk page at the old name redirects to the right talk page. Even if the user is renamed multiple times, someone would have fixed the double redirect. This is also the original ask at #90. |
|
Sorry this dropped off my radar
That would work for most renames yes, but we also end up with cases where a user might request deletion of their previous usernames due to personal information concerns (which I assume is more amongst new users due to banner blindness on registrations). This would also handle those cases as well. |
|
In the case of a non-renamed user having a user talk that redirects somewhere else, would this patch follow the redirect? (I think it should. Appending AFC messages to a redirect page is bad.) I agree with siddharthvp's concerns. If we can solve the problem by adding a parameter to an existing API query, I think that's preferable to using complicated recursion. Could still be convinced otherwise though, but that's my initial thought. |
|
It probably wouldn't but then again, that could be solved by including redirect:true in addition to this change. However, I'm somewhat unconvinced that the case (redirecting user talk pages without renames) is going to be prevalent amongst new users, compared to the case of creating a user account with the same name as a company and then requesting a rename and a deletion of the previous username's redirects to prevent association. If we do go with only checkinh redirects, that would not address the deleted redirect scenario and instead create user talk pages for users that don't exist. |
|
I suspect the usual workflow would be softblock because of organizational or role name -> user follows directions and requests name change -> name is changed, with the name changing process creating redirects. I doubt they would know how to request deletion of the redirects nor would they be motivated to. Want to go ahead and make the little changes suggested above so this is easy to merge if we decide to keep the current approach? |
Fixes wikimedia-gadgets#90 Advantages - Simpler than patch wikimedia-gadgets#291 - The original intent of the function notifyUser was to follow redirects, indicated by the comment "Follows redirects and appends a message to the bottom of the user's talk page." from 2014. So this fixes that bug. Disadvantages - Will only follow 1 redirect, not 2+ redirects. - The "Saved [[User talk:OldUsername]] (diff)" message shown to the AFC reviewer will be slightly incorrect. The diff will be correct, but OldUsername should ideally be NewUsername.
NovemLinguae
force-pushed
the
renamed-user
branch
from
d8c87f8 to
ab7abf3
Compare
|
Tested, works. Primefac supports merging this version. Will merge this on Monday unless there are comments over the weekend. |
This code does NOT follow redirects as currently written. Patch wikimedia-gadgets#291 will handle this a different way.
Fixes #90 Advantages - Simpler than patch #291 - The original intent of the function notifyUser was to follow redirects, indicated by the comment "Follows redirects and appends a message to the bottom of the user's talk page." from 2014. So this fixes that bug. Disadvantages - Will only follow 1 redirect, not 2+ redirects. - The "Saved [[User talk:OldUsername]] (diff)" message shown to the AFC reviewer will be slightly incorrect. The diff will be correct, but OldUsername should ideally be NewUsername.
Fixes #90