fix: disable vote option while loading

[hamza221]

Without this you can run into this bug

[ChristophWurst]

Looks good!

[AndyScherzinger]

LGTM, yet let's see what @dartcafe feedback will be.

[dartcafe]

I can't reproduce it. How did you run into this situation?

[hamza221]

I can't reproduce it. How did you run into this situation?

It happens when you double click a vote option ( spamming also works)

[dartcafe]

I klicked like a maniac, but the error did not raise, even if I simulate a GPRS bandwidth.

But your solution has the side effect, that it lets the background flash unintenionally.

click.flashing.mp4

I see this issue as not necessary. It would be more valuable, if it would be applied to the next branch, because this branch will be a dead end. Next will replace master as future main branch.

[dartcafe]

BTW: Any race condition should be solved by lodash/debounce and request canceling inside the api, so that only the last vote within the debounce timespan should be accepted.

If this does not work for any reason, the fix should be applied there. Blocking the vote cell is not a good solution, since it is a bad UX.

[hamza221]

I klicked like a maniac, but the error did not raise, even if I simulate a GPRS bandwidth.

Screen.Recording.2024-08-30.at.13.17.52.mov

As you can see in the video the reproduction is pretty straight forward, You just have to double click a vote option
You don't have to simulate a GPRS bandwidth.

since it is a bad UX.

I'm sorry but I don't really understand your argument here. I think that the way it is right now is a bad UX (wrong warning message, and allowing a button press when you shouldn't )

I really think that a good UX in this case is

1. Press Button
2. Wait for action to fulfil
3. allow action again

that it lets the background flash unintenionally.

It is intentional, it indicates that the button is disabled
If you think a loading indicator may help in this case we can add it

[dartcafe]

First of all: I am still not able to provocate the 409.

But there is no reason to disable the current vote cell, with only one exception: The user already has more yes votes than allowed. In this case changing the vote from "yes" to "no" disables the current vote cell.

A race condition, caused by clicking while the request is still ongoing, should be captured by loadash/debounce or by cancelling previous request. In the result only the last vote is to be persisted.

This is, why I want to understand, under which circumstances the 409 occurs, because it should not.

[dartcafe]

Buut. Luckily after "a million" clicks and getting the exact click frequence needed, I now managed to get the 409 and see what happened. Wow, this is a very rare edge case with my setup.

Explanation:
The 409 is raised, because a yes vote is sent, but the server says, the vote limit is already reached, because this particular vote is already stored as "yes" and is therefore blocking the vote.

Currently this error is handeled by capturing it and reload the votes and the options to get a consistent state. But here we also have to reload the poll information, because the information about the remaining votes went there.

polls/src/js/store/modules/votes.js

Lines 107 to 116 in 54334fe

	} catch (error) {
	if (error?.code === 'ERR_CANCELED') return
	if (error.response.status === 409) {
	context.dispatch('list')
	context.dispatch('options/list', null, { root: true })
	} else {
	Logger.error('Error setting vote', { error, payload })
	throw error
	}
	}

Now we can follow two strategies:

1. Avoid the 409 at the server side or

2. handle the error better at the client side, because now we have a misinformation about the remaining votes.
   The vote has the "yes" state, vote limit is set to "1 vote per user", but the card says "1 vote left", which is false.

3. The vote is meant to get the state "yes", but it already has this state. This means we should skip the limit check and just commit the vote and return a response as if the vote has been set sucsessfully.

polls/lib/Service/VoteService.php

Lines 71 to 85 in 54334fe

	public function set(int $optionId, string $setTo): ?Vote {
	$option = $this->optionMapper->find($optionId);
	$poll = $this->pollMapper->find($option->getPollId());
	$poll->request(Poll::PERMISSION_VOTE_EDIT);
	if ($setTo === Vote::VOTE_YES) {
	$this->checkLimits($option);
	}
	// delete no votes, if poll setting is set to useNo === 0
	$deleteVoteInsteadOfNoVote = in_array(trim($setTo), [Vote::VOTE_NO, '']) && !boolval($poll->getUseNo());
	try {
	$this->vote = $this->voteMapper->findSingleVote($poll->getId(), $option->getPollOptionText(), $this->acl->getCurrentUserId());
	if ($deleteVoteInsteadOfNoVote) {

2. Also reload the poll here additionally to the votes and options

polls/src/js/store/modules/votes.js

Lines 107 to 116 in 54334fe

	} catch (error) {
	if (error?.code === 'ERR_CANCELED') return
	if (error.response.status === 409) {
	context.dispatch('list')
	context.dispatch('options/list', null, { root: true })
	} else {
	Logger.error('Error setting vote', { error, payload })
	throw error
	}
	}

[dartcafe]

And thinking further about it, I believe both strategies should be applied.

1. capture the situation, where the vote is meant to be set to the state it already has at the server side
2. load the poll configuration due to consistency reasons.

A further optimization would be to avoid two requests, by directly return the poll and options upon voting.

I will directly add a commit, since I currently have the particular code opened.

[dartcafe]

What I did:

1. skip any action, if the stored vote already has the same state as it is intendet to be changed to
2. revert your changes
3. Add poll loading if 409 should appear to secure consistency
4. Just optimization: Consolidate the three requests into one by returning poll and options list inside the vote request

This should avoid the 409 error in this cases, avoid some additional requests and secures a better UX, because the vote cell gets not blocked by fast clicking.

[dartcafe]

I'm sorry but I don't really understand your argument here. I think that the way it is right now is a bad UX (wrong warning message, and allowing a button press when you shouldn't )

Since there wasn't any warning displayed to the user, I don't see any UX issue. What warning are you referring to? The bad UX was the misinformation about the left votes, which was wrong (you can see it in the last seconds of your video).

I really think that a good UX in this case is
1. Press Button
2. Wait for action to fulfil
3. allow action again

And this is exatly what I intended to avoid and understand as a bad UX, since blocking prior votes is unnecessary. Only my last vote counts.

For example in case of a laggy connection, it may be annoying, if I have to wait for the responses which are simply no more valid, to be able to set my decision of the vote. Therefore prior unfulfilled votes get discarded instead of waiting for the commits.

[dartcafe]

@hamza221 I tested the changes quite some time:

• I couldn't provocate the 409 anymore
• The vote cell stays clickable as intended (I think this is an information, that you were missing)
• Two requests get avoided after voting.

Do you want to review that?

[hamza221]

I can't reproduce anymore ✅

[dartcafe]

Just a recap: Understandable, why this happened. 💡

If the db state is 'yes' and only the last 'yes'-vote gets through (prior votes get discarded) the exception gets triggered, if the allowed number of remaining votes is 0.

Then the limit check returns the exception, regardless if the vote does not change the number of used votes.

Anyway strange I could not reproduce it from the start.

[AndyScherzinger]

Yes, has been quite a rabbithole to jump into 👍