Skip to content

Add JavaScript verification to Assigner page. - #3226

Open
somiaj wants to merge 1 commit into
openwebwork:developfrom
somiaj:js-confirm-unassign
Open

somiaj wants to merge 1 commit into
openwebwork:developfrom
somiaj:js-confirm-unassign

Conversation

@somiaj

@somiaj somiaj commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

This adds JavaScript verification which checks that at least one user and one set is selected. The warning about unassigning sets is moved to the help section. When at least one set and user are selected, the unassign button will open a confirmation model warning the user about data loss and listing what users will be assigned from what sets.

There are more pages to work on, but I am focusing on a single page for this PR.

@drgrice1 Claude's review of this change was completing removing the server side confirmation could be dangerous. If the JavaScript fails or is bypassed through some sort of reload (though I don't see how), there is no guard server side. I'm unsure if this is an issue we should care about, but what about having a hidden input that is updated vs just a JavaScript variable that is sent as part of the confirmation?

@drgrice1

Copy link
Copy Markdown
Member

It should be easy to add a parameter that is appended to the submission by javascript when the confirmation occurs.

There is something wrong with this though. This is blocking submissions when the "Refresh List" buttons of the scrolling record lists are pressed when no users or sets are selected. Users and sets should not need to be selected for that.

@drgrice1

Copy link
Copy Markdown
Member

Furthermore, a button should not be required. With the develop branch you can change the parameters of the scrolling records lists and hit enter, and the lists will update. That should still be the case.

@somiaj
somiaj force-pushed the js-confirm-unassign branch from b827ad8 to 0b65341 Compare September 30, 2026 14:15
@somiaj

somiaj commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

I missed that the Refresh button was a submit. I wonder if that should be updated to use JavaScript instead.

Anyways, I fixed it so the refresh submits get through properly. I will think about adding a hidden element to pass to give some server side verification (though if we go this route, should we update your PRs to do the same?) I'm unsure how useful it is, but I guess it won't hurt.

The only place that enter 'refreshes' the list for me is when I have selected the union/intersection radio buttons. All the selects/dropdowns enter doesn't hit refresh.

@somiaj
somiaj force-pushed the js-confirm-unassign branch from 0b65341 to 626dd17 Compare September 30, 2026 14:20

@drgrice1 drgrice1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that this looks good at this point.

However, I am uncertain what you meant with the comment "Claude's review of this change was completing removing the server side confirmation could be dangerous." Could you state what that really meant?

@somiaj

somiaj commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Opps, spell check got the wrong word, I meant "was contemplating". Basically Claude mentioned that if the javascript were to not load / was disabled, then the submit button could be hit without confirmation, since the server side check was removed. Claude also mentioned the possibility of a page refresh sending post data again.

Sending a hidden field that javascript toggles could help in the case javascript doesn't load correctly or is disabled.

I am unsure if we should worry about that possibility, and was just pointing out Claude thought that removing the server side check could be dangerous. Your other PRs didn't worry about it, so I only wanted to mention it but not do anything about it.

@drgrice1

drgrice1 commented Oct 2, 2026

Copy link
Copy Markdown
Member

That is not a bad point. It wouldn't hurt to protect against a submission. I added hidden inputs to #3223 whose value is set by javascript when confirmation is obtained from the user. That value is checked server side and the deletion is refused if it is not set.

@somiaj
somiaj force-pushed the js-confirm-unassign branch from 626dd17 to b954fc5 Compare October 3, 2026 01:49
@somiaj

somiaj commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

I added a hidden field to give a layer of confirmation on the server side here too.

@somiaj
somiaj force-pushed the js-confirm-unassign branch from b954fc5 to ff03db0 Compare October 4, 2026 14:37
@somiaj

somiaj commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Claude pointed out "is not undoable" was poorly worded, so I updated this to be "cannot be undone".

@drgrice1

drgrice1 commented Oct 4, 2026

Copy link
Copy Markdown
Member

I don't know that "is not undoable" is any more or less clear than "cannot be undone". I think that is tit for tat.

@somiaj

somiaj commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Since the "is not undoable" is common language in other parts of the interface, I should probably be consistent with that.

I agree with you that it is tit for tat, but both Claude and Gemini both consider "cannot be undone" more direct because it doesn't require parsing a double negative. For consistency I'll go update these to match other pages (unless there is a desire to update the language elsewhere too, which I don't have).

@drgrice1

drgrice1 commented Oct 5, 2026

Copy link
Copy Markdown
Member

Wow, Claude and Gemini are really both that stupid. Both statements not only both have double negatives, they both have the same two negatives "not" and "un". Here are the two statements emphasizing the two negatives "is NOT UNdoable" and "canNOT be UNdone".

@somiaj
somiaj force-pushed the js-confirm-unassign branch from ff03db0 to fbb601a Compare October 6, 2026 15:16
This adds JavaScript verification which checks that at least
one user and one set is selected. The warning about unassigning
sets is moved to the help section. When at least one set and
user are selected, the unassign button will open a confirmation
model warning the user about data loss and listing what users
will be assigned from what sets.
@somiaj
somiaj force-pushed the js-confirm-unassign branch from fbb601a to 8cc6f72 Compare October 6, 2026 18:10

@drgrice1 drgrice1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good.

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.

2 participants