Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

users should not be able to add new options shortly before poll ends … #263

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

Held99
Copy link
Contributor

@Held99 Held99 commented Nov 6, 2023

Users now shouldn't be able to add option after 1/14 of voting time has passed. The add option buttons are greyed out and red warning message is shown.

@Held99 Held99 requested a review from mensch72 as a code owner November 6, 2023 20:40
@ghost
Copy link

ghost commented Nov 6, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

Copy link
Collaborator

@mensch72 mensch72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The code looks fine. Haven't tested it yet but will tomorrow.

One small request: Could you remove the added English text entries from the non-English language files again, please? If they are missing, the fallback language (English) will automatically be used. If they are there, our translation tool will think they have already been translated.

@mensch72
Copy link
Collaborator

mensch72 commented Nov 6, 2023

OK, I tested it now. The "+" button on the poll page is correctly disabled as soon as the time threshold is crossed. That is good!

But the error message that I was asking for is showing up on the poll page now rather than in the addoption dialog popup. I am sorry that I was not clear enough regarding the error message. Let me clarify:

  • On the poll page, we don't need such a big message. It suffices that the "+" button is disabled. Maybe right next to that button one could have a small message, I would be OK with that and leave that to you. But not as large as right now please.
  • More importantly, the missing thing is the following: If the user has already opened the add option dialog (i.e., has already pressed the button) before the time threshold is crossed, and then the time threshold gets crossed while the add option dialog is still open, before the "OK" button is pressed, then the "OK" button should also be disabled and a red error message below that button should appear, formatted in the same style as the messages produced by the form validators below the form fields. Otherwise, a user would be able to add an option very shortly before the poll's deadline by opening the dialog early on and then pressing the OK button very late.

Do you think you could still make these changes? Sorry again for not having been clear before...

@Held99
Copy link
Contributor Author

Held99 commented Dec 4, 2023

Hi, Sorry for getting back to you so late, I think I changed what you requested. Can you take a look @mensch72 ?

@mensch72
Copy link
Collaborator

No need to be sorry, I have to be actually, for not getting back earlier :-) At first glance it looks well. I will look at this in more depth before the end of the week. Thank you again for your work!

@mensch72 mensch72 merged commit bed97c8 into pik-gane:main Dec 19, 2023
2 checks passed
@mensch72
Copy link
Collaborator

OK, I tested it. There was still a minor problem when the addoption dialog was already open but the user not doing anything when the adding deadline is reached, because then the javascript code gets inactive and only detects the passing of the deadline when it is resumed at the next user action; if that next user action was clicking the "OK" button (which was not greyed out at that moment), then the adding went through because the pressing was processed before the button then was greyed out. I have merged this PR now and fixed the remaining thing myself in a separate PR. Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants