Hi @vaughnwalters, thanks for reporting this bug!
I was checking on it and I found other bugs in the two scenarios listed below.
Please let me know if the scenarios and bugs below are correct (I mean, are they really bugs?).
cc: @ifried, @vyuen

@Daimona, @MHorsey-WMF, could you please take a look at my proposal of how to fix the bugs on each scenario, and let me know what you think?

Scenario 1: Select all participants and then deselect one or more, and scroll to load more participants
Given the organizer is on Special:EventDetails
And the organizer select all participants
And the organizer deselect one or more participants
If the organizer scroll down to load more participants
Then the returned participants should appear as checked (selected)

Bug Found:

• The loaded participants when scrolling come as not selected.
• They should come as selected because the user clicked on select all, and all means all the participants, even the ones not loaded yet if the user did not scroll down.

Proposed solution:
when loading more participants (scroll down)
If this->selectAllIsActive = true
Then the participants should appear as selected

Obs: this->selectAllIsActive would be a new property set as true when the organizer clicks to select all, and set as false when the organizer clicks to deselect all

Scenario 2: Select all participants and then deselect one or more and try to remove participants
Given the organizer is on Special:EventDetails
And the organizer select all participants
And the organizer did not scroll down to load the rest of the participants
And the organizer deselect one participant
And there is a total of 40 participants (only 20 loaded)
When the organizer clicks on remove participants
Then the list of selected user IDs sent to the backend should have 39 user IDs not 19

Bug Found:

• For this scenario we should send 39 participants to be removed, but because the organizer did not scroll down, we send only 19.

Proposed solution:
Send the list of NOT selected users instead of the selected users
Change the query on the backend to:

• delete from ce_participants where ce_user_id not in (not_selected_user_ids)

		e.g:
			1 - Selected
			2 - Not selected
			3 - Not selected
			delete from ce_participants where ce_user_id not in (2,3)

		Obs: if the organizer deselect all we don't need to worry because the remove button is hidden, but we should add a validation on the PHP side as well to avoid this properly

Sorry for the long comment, please let know if this is clear enough, thanks!

We will estimate this one asynchronously

From the task description:

Scroll to the bottom of the infinite scroll so that all particpants are loaded in. Click select all. Deselect one of the participants. You will see that the overall count is off by one. In this example below, after deselecting one participant, the count selected shows 46, but the total count shows 48.

This is actually working as intended, although it's a bit surprising. The event you're using for testing has a hidden/deleted account amongst its participants:

daimona@deployment-deploy03:~$ mwscript shell.php --wiki=enwiki
Psy Shell v0.11.8 (PHP 7.4.30 — cli) by Justin Hileman
>>> $partStore = \MediaWiki\Extension\CampaignEvents\CampaignEventsServices::getParticipantsStore();
>>> $partStore->getParticipantCountForEvent( 118 )
=> 48
>>> $participants = $partStore->getEventParticipants( 118 );
>>> $userLookup = \MediaWiki\Extension\CampaignEvents\CampaignEventsServices::getCampaignsCentralUserLookup();
>>> foreach ( $participants as $p ) { if ( !$userLookup->existsAndIsVisible( $p->getUser() ) ) echo "Hidden", PHP_EOL; }
Hidden

So while there are indeed 48 participants, only 47 are shown in the interface and can be selected. I do think this should be fixed, but probably in a separate task, as it has a different root cause than the other bugs reported here. I had filed T313492 a few months ago, I'm unsure if that task is enough or whether we need a new one.

In T320649#8330963, @cmelo wrote:

Scenario 1: Select all participants and then deselect one or more, and scroll to load more participants

Indeed, this is a bug.

Proposed solution:
when loading more participants (scroll down)
If this->selectAllIsActive = true
Then the participants should appear as selected

Obs: this->selectAllIsActive would be a new property set as true when the organizer clicks to select all, and set as false when the organizer clicks to deselect all

That's doable, but not as easy as it may seem. If you click "select all" then unselect one, the "select all" checkox is no longer checked; therefore, if you want to really deselect all participants you have to click the checkbox twice. We would also need to set the property to false if the user scrolls down until the very last checkbox, and deselects all checkboxes manually.

Scenario 2: Select all participants and then deselect one or more and try to remove participants

And this is a bug as well.

Proposed solution:
Send the list of NOT selected users instead of the selected users

Yeah, that might work, but I wonder if it's worth the added maintenance cost.

I actually have an alternative proposal that would probably fix all (?) these bugs, or at least make the code easier to deal with: change the functionality of the "select all" button. My strawman proposal would be:

• Make the "select all" checkbox select all visible participants
• Have separate buttons for "remove selected" and "remove all". Same for every other action that you can take after selecting participants (e.g., messaging).
  • The "X all" button should only be included if there's a concrete use case for it. (Which might be the case here, what I mean is: don't add it only for symmetry, if it's useless)
• If user clicks "select all" and then scrolls, new participants are not selected by default
• In fact, maybe the "select all" button should not be a checkbox at all. Maybe it could be some other control that does not remain toggled after it's clicked.
  • At that point, the label for the new "select all" control could be separated from the "X selected" label, addressing a concern raised by @ifried in the last paragraph of T320635#8312728.

In general, I think combining infinite scrolling with a "select all" (where "all" means really everyone) may not be good UX after all. Some things are fairly hard to implement (as this task demonstrates), and the behaviour may still be surprising.

Change 850088 had a related patch set uploaded (by Cmelo; author: Cmelo):

[mediawiki/extensions/CampaignEvents@master] Fix participants list selected

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

Test wiki created on Patch demo by CmeloV using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/c75d677517/w

Change 854620 had a related patch set uploaded (by Cmelo; author: jenkins-bot):

[mediawiki/extensions/CampaignEvents@master] Fix participants list selected

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

Change 854620 abandoned by Cmelo:

[mediawiki/extensions/CampaignEvents@master] Fix participants list selected

Reason:

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

Change 850088 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Fix participants list selected

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

Change 857841 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Fix more bugs in participant selection on EventDetails

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

@vaughnwalters Some notes for when this will be in QA: aside from the bug mentioned in the task description, there were multiple bugs found while developing and reviewing. I actually lost count of them, so I don't have a list for you. But I don't think we need one... My only suggestion would be: test this as much as you can. Any combination of the following things would be worth testing:

• Current user is organizer or not
• If current user is organizer, are they also a participant or not
• Select all
• Select all then unselect some
• Select users manually, with and without having clicked "select all" before
• All the things above should also be tested with search. Examples could be:
  • Select all, then search and see if the selection is remembered
  • Select users manually, then search and see if the selection is remembered
  • Search, select, delete search term, see if the selection is remembered
• Select users in some way, then remove them, then select other users and remove them again without reloading the page first
• Scroll down to load more users and verify that they're (un)selected depending on the state of the "select all" checkbox
• Only the first batch of users is visible vs all users were loaded and are visible

And possibly more... There are lots of scenarios and edge cases, and I'm not sure if we have a list of all of them, so the more you can test, the better. Some things to look for are:

• Number of participants is reported correctly, both in the initial header ("X participants") and in the "select all" checkbox label ("X out of Y selected")
• Checkboxes remain (un)selected when they should, e.g. when searching
• When removing participants, the numbers are updated correctly

And again, this is not the full list... Just try everything that you can think of, and if anything looks suspicious or confusing, you can probably assume it's a bug that we didn't know about...

Change 857841 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Fix more bugs in participant selection on EventDetails

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

Should be ready for testing now... Notes are in T320649#8401175.

Log in as the organizer of the event
Register through Special:RegisterForEvent/{id}
Navigate to the participant tab
select and delete yourself.
the participant count is correct
the $num of $num selected is incorrect

When the page is refreshed however, all of the counts are correct.

In T320649#8411675, @vaughnwalters wrote:

Log in as the organizer of the event
Register through Special:RegisterForEvent/{id}
Navigate to the participant tab
select and delete yourself.
the participant count is correct
the $num of $num selected is incorrect

When the page is refreshed however, all of the counts are correct.

Actually this isn't just for deleting yourself, this bug applies to when deleting any user. but the count is corrected as soon as you refresh or click on any checkbox:

Change 859154 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] EventDetails: update selection label after removing participants

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

Yup, confirmed, and the fix is up.

Change 859154 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] EventDetails: update selection label after removing participants

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

In T320649#8411688, @Daimona wrote:

Yup, confirmed, and the fix is up.

That fix is now working correctly. See gif below:

I have done my best to continue to try to break this, but alas, I can not currently find any more errors. 👏 👏 .

Moving this to product sign off.

Test wiki on Patch demo by CmeloV using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/c75d677517/w/