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

Fixed issue #18693: Using checkbox question for yes/no loses data when data entry view #3018

Conversation

gabrieljenik
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@Shnoulle Shnoulle left a comment

Choose a reason for hiding this comment

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

Since unseen can be used as subquetiontitle, maybe use a different separator :unseen can work i think.

See : https://www.w3.org/TR/html401/types.html#type-name

application/controllers/admin/DataEntry.php Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Mar 29, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Shnoulle Shnoulle added Code review done Version checked for code issue without testing and removed Needs code review labels Mar 30, 2023
@Shnoulle
Copy link
Collaborator

The question still exist «why not {$fname['fieldname']}:unseen » ?

@gabrieljenik
Copy link
Collaborator Author

The question still exist «why not {$fname['fieldname']}:unseen » ?

I believe the : can be error prone or problematic.
The current naming schema works. So, don't want to add an extra layer of "risk" (as a way to say it) :)

@Shnoulle
Copy link
Collaborator

Shnoulle commented Apr 4, 2023

I believe the : can be error prone or problematic.
The current naming schema works. So, don't want to add an extra layer of "risk" (as a way to say it) :)

You think i write : without controlling it's OK ?

ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods (".").

Source : https://www.w3.org/TR/html401/types.html#type-name

@gabrieljenik
Copy link
Collaborator Author

You think i write : without controlling it's OK ?

No, of course not. Why do you take it like that.
I am just saying it is not very usual to use : on names, so prefer to not use it.

We can disagree sometimes, right? If at the end it works....

@Shnoulle
Copy link
Collaborator

Shnoulle commented May 5, 2023

PS : strange when i try to test : seems to be a 5.X

@Shnoulle
Copy link
Collaborator

Shnoulle commented May 5, 2023

No, of course not. Why do you take it like that.

Because you don't tell why you don't want it …

I am just saying it is not very usual to use : on names, so prefer to not use it.

Its one of the reason to use it, you have less chance to have it in a name. And it's make clear that is a new name.

In LimeSurvey : we already use _ as separator between code and subquestion code, the when see _ : a new xdev can think it's a separator between code and subcode.

Another : there are discussion since years to use QuestionCode in place of sidXgidXqid, with unseen at start with _ : you disable usage of unseen.

We can disagree sometimes, right? If at the end it works....

Yes, but maybe discuss on idea is a good solution to have something work for long term and more understandable

@olleharstedt
Copy link
Collaborator

Right, both underscore and hash are used in question code generation? Or is that SGQA code only?

Where in the code is the current naming schema decided? Can't find it in the DataEntry class.

@Shnoulle You have an example that could break the current schema?

@olleharstedt
Copy link
Collaborator

olleharstedt commented May 5, 2023

name='unseen_{$questionInputField}'

This one right? Pja, can it break if someone creates a question with code "unseen" and then subquestions?

Double underscore is another alternative, maybe. Or name=unseen[$questionCode]

@gabrieljenik
Copy link
Collaborator Author

Waiting on approavl of #3109 before updating this one.

@gabrieljenik
Copy link
Collaborator Author

Picked up. Ready for re-reviewal

Copy link
Collaborator

@Shnoulle Shnoulle left a comment

Choose a reason for hiding this comment

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

Please : don't loose data sent

(i didn't review all currently)

application/controllers/admin/DataEntry.php Show resolved Hide resolved
@gabrieljenik
Copy link
Collaborator Author

Please : don't loose data sent

We set JS validation, just like the other branch...

@Shnoulle
Copy link
Collaborator

Please : don't loose data sent

We set JS validation, just like the other branch...

Don't send a 400 error in case JS broken !

If JS broken : save data but send an flash alert.

@Shnoulle
Copy link
Collaborator

Please : don't loose data sent

We set JS validation, just like the other branch...

About other branch : this one is the 1st done, i didn't see the 400 error before sorry.

…n data entry view

- Fix bug when last question has subquestions ( === false)
- Don't throw exception when Unseen is checked and a value is provided
…n data entry view

- Fix file changed by mistake
@Shnoulle Shnoulle added Code review done Version checked for code issue without testing and removed Needs code review labels May 26, 2023
@gabrieljenik gabrieljenik added Tested OK This PR has been tested by QA and works as expected and removed In Testing labels Jun 8, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jun 13, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 25 Code Smells

0.0% 0.0% Coverage
10.3% 10.3% Duplication

@tiborpacalat tiborpacalat merged commit 3ee692c into master Jun 14, 2023
@tiborpacalat tiborpacalat deleted the bug/18693--Using-checkbox-question-for-yes-no-loses-data-when-data-entry-view branch June 14, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code review done Version checked for code issue without testing Tested OK This PR has been tested by QA and works as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants