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 #18181: "no answer" still shown after turning it off #2700

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

Shnoulle
Copy link
Collaborator

@Shnoulle Shnoulle commented Nov 3, 2022

Dev: remove deprecated usage of App()->getConfig('shownoanswer')
Dev: No GUI to set : no way to update, remve it form config-default too

Dev: remove deprecated usage of  App()->getConfig('shownoanswer')
Dev: No GUI to set : no way to update, remve it form config-default too
@sonarcloud
Copy link

sonarcloud bot commented Nov 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@gabrieljenik
Copy link
Collaborator

gabrieljenik commented Nov 3, 2022

Is it necesarry to also remove the config-defaults?
Isn't this also a survey group setting?

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Nov 3, 2022

Yes :
in 3.X : we have the settings in GUI
in 5.X : no settings in GUI

If you remove usage : no need to keep it config-default : not used anymore.

Alternative solution : add the settings in GUI AND disable settings in Survey Settings (and group settings).

@gabrieljenik
Copy link
Collaborator

gabrieljenik commented Nov 3, 2022

If you remove usage : no need to keep it config-default : not used anymore.

Not sure I follow. Why is not used anymore?

Ok, is not in the GUI anymore.
Still, if you keep the config-defaults, you can set the config globally through the file.

Maybe it is in the Olle/Carsten's plan to remove it.
Still, I haven't read that anywhere.
What if someone uses it?
So, removing it now, on a minor release, seems too much.

I think this is fixable keeping the config-defaults.
Later, we can remove it.

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Nov 4, 2022

There are 3 solutions :

  1. Remove it because it must be removed at some time but someone didn't remove it
  2. Leave it and the,n fix all other part (add global settings, show settings as disable, show help for simple user etc …)
  3. Leave it but without allowing to update in global settings (need show settings as disable, show help for simple user etc …)

Maybe it is in the Olle/Carsten's plan to remove it.
Still, I haven't read that anywhere.

The number of things updated without clear decision during each big update are very high …

What if someone uses it?

See the reported issue : we need 5 month to understand what happen. I have the issue for a friends of mine to understand what happen. Friend ask me to hide it, superadmin user … 3 reporters have the same issue etc …

I set as decision needed @olleharstedt and @c-schmitz : keep the global settings or remove it ?

We have same issue with 3 other global settings https://bugs.limesurvey.org/view.php?id=18450

@Shnoulle Shnoulle added the Decision needed Use when a discussion does not lead to consensus, to get a decision from Carsten/Olle/other label Nov 4, 2022
@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Nov 4, 2022

I think the Global survey settings replace this part.

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Dec 8, 2022

@olleharstedt need to add the global settings (like in 3.X)
Or Global survey settings replace it ?

@olleharstedt
Copy link
Collaborator

Dunno, man. We need bi-weekly meeting for stuff like this.

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Dec 9, 2022

Here : issue reported : 2022-06-07 12:24
Find a way to reproduce (by a LS dev) : 2022-11-02

3 user on same mantis with discussion and say "we have same issue"
3 user report issue mean a lot more have issue ;)

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jan 5, 2023

Still people who can not deactivate "Show no answer" …

@Shnoulle
Copy link
Collaborator Author

Still people who can not deactivate "Show no answer" …

@tiborpacalat tiborpacalat added Needs code review Needs testing and removed Decision needed Use when a discussion does not lead to consensus, to get a decision from Carsten/Olle/other labels May 30, 2023
Copy link
Collaborator

@gabrieljenik gabrieljenik left a comment

Choose a reason for hiding this comment

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

Code looks ok, not tested

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 6, 2023

Need to be in 5

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

Shnoulle commented Jun 9, 2023

@olleharstedt : what is the decison here ?

See https://bugs.limesurvey.org/view.php?id=18181 3 user minimum.

The issue is about removing this option from GUI but leave the test. Then it's a specific way for reproduce.

But clealy need a cleaner solution

I need to fx it 5, but if it's another decision : i wait.

@olleharstedt
Copy link
Collaborator

The issue is about removing this option from GUI but leave the test

Hm you want to remove the question attribute? GUI = question editor?

@olleharstedt
Copy link
Collaborator

If both Gabriel and Denis thinks this solution is OK, then it's OK for me. :) Just waiting for testing to happen then?

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

Hm you want to remove the question attribute? GUI = question editor?

NO : Global settings : « Show 'no answer' option for non-mandatory questions:

Capture d’écran du 2023-06-09 11-27-02

Disappear in 5.X from GUI

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

If both Gabriel and Denis thinks this solution is OK, then it's OK for me. :) Just waiting for testing to happen then?

I'm unsure if we have same decision; I don't understand oif @gabrieljenik want to keep the global settings «« Show 'no answer' option for non-mandatory questions:» in 5 (and then add it again to GUI) or if it's another solution.

@olleharstedt
Copy link
Collaborator

Disappear in 5.X from GUI

On purpose? Or bug? Was a decision made?

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

Disappear in 5.X from GUI

On purpose? Or bug? Was a decision made?

You really ask to me ?
07c3a94

Like here : #3157

@gabrieljenik
Copy link
Collaborator

If both Gabriel and Denis thinks this solution is OK, then it's OK for me. :) Just waiting for testing to happen then?

Honestly, don't recall the issue. But is not that Denis and I got aligned, but more that a decision was done by someone else [https://bugs.limesurvey.org/view.php?id=18181#c75418]. That's why I mvoed forward with the code review.

I am picking this app, so I maynot recall details.
My thought:

  • If the setting dissapeared from GUI
  • but is still used by scripts
  • Then
    • reinstante the GUI is better than
    • removing the setting at global al level and also at config.php level.
  • Why?
    • Not loose feature
    • Less impact on the code and on the app

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

I think because the system move to [inherit] by global Survey settings (and SurveyGroup settings).

We don't have global survey settings before.

And here : it lack a default value.

We can add it again but then need to show it at disable a all survey settings (global + SurveyGroup)

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

  • Less impact on the code and on the app

What ?

The issue still persist about Surey settings : we must show as disable.

More code to do

@gabrieljenik
Copy link
Collaborator

We can add it again but then need to show it at disable a all survey settings (global + SurveyGroup)

Chain goes: Config -> Global ->Survey Group -> Survey
Right?
Wouldn't the survey settings override the global?

The issue still persist about Surey settings : we must show as disable.

OK, yes, probably something on the survey groups and survey setting shall be reviewed.
But it would be consistent with other settings, right?

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

Chain goes: Config -> Global ->Survey Group -> Survey

No : Config > Global survey (direct)
Capture d’écran du 2023-06-09 15-07-42

Wouldn't the survey settings override the global?

Inherited value.

We have issue reported because user CAN NOT update this part. No issue because this settings disappear to force Survey settings.

But it would be consistent with other settings, right?

No : it's the only Survey settings that can be forced to ON or OFF. There are no oither settings …

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

@gabrieljenik if we add it again : why not

Show "There are X questions in this survey":
Capture d’écran du 2023-06-09 15-12-00

Show question group title and/or description:
Capture d’écran du 2023-06-09 15-12-33

Show question number and/or question code:
Capture d’écran du 2023-06-09 15-12-59

Maybe because these parameters have been completely removed without leaving out a single part?

@gabrieljenik
Copy link
Collaborator

OK, we have a mixed situation.
What's the future desired direction? @tiborpacalat ?

Set it up from global settings or global survey?

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 9, 2023

Set it up from global settings or global survey?

It's currently in global survey, the question is not OR.

The questions are

  1. «Do we want to force user with GlobalSettings permission to force some survey settings»
  2. If yes : why only this one and all other (the 3 previous one for started)

@gabrieljenik
Copy link
Collaborator

I am sorry. I can't follow the discussion.
Seemes there is something I am missing and believe that now that Tibor and Olle are involved, I can just retire from the topic. :)

@tiborpacalat
Copy link
Collaborator

I don't know why there is still discussion about this. I went through tickets that had "Decision needed" with Carsten and this is why the tag was removed. After that I commented that the PR is fine and should be reviewed and tested. If you want additional things to be handled, please create a new ticket.

@tiborpacalat
Copy link
Collaborator

This PR points to master, but it is actually for 5.x.
Please update this branch for master and create another one for 5.x.

@Shnoulle Shnoulle changed the base branch from master to 5.x June 9, 2023 13:40
@tiborpacalat tiborpacalat added the Test failed Someone tested the PR and it did not work as expected. Please check out the comments. label Jun 9, 2023
@Shnoulle
Copy link
Collaborator Author

Fixed the save issue

And clearly remove comments about global settings are off on shownoanswer , showxquestions , showgroupinfo , showqnumcode.

The questions now are (or @tiborpacalat and @olleharstedt )

  1. Did i create same PR for master
  2. OR did i add shownoanswer choose/forced on global settings like 3.X
  3. OR i didn't add it at global settings BUT : show it as not editable on Survey settings and at SurveyGroup settings (with the reason)
  4. MUST test for the 3 other settings if it work (and remove it from config-default if not.

@Shnoulle Shnoulle force-pushed the bug/18181_shownoanswer branch 2 times, most recently from dc00bb5 to 8dfb6dc Compare June 12, 2023 08:33
Dev: remove outdated comments about global settings off
@sonarcloud
Copy link

sonarcloud bot commented Jun 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Shnoulle Shnoulle removed the Test failed Someone tested the PR and it did not work as expected. Please check out the comments. label Jun 12, 2023
@tiborpacalat
Copy link
Collaborator

All settings are working shownoanswer , showxquestions , showgroupinfo , showqnumcode.

@tiborpacalat tiborpacalat merged commit 908a508 into 5.x Jun 13, 2023
11 checks passed
@tiborpacalat tiborpacalat deleted the bug/18181_shownoanswer branch June 13, 2023 13:53
@Shnoulle
Copy link
Collaborator Author

All settings are working shownoanswer , showxquestions , showgroupinfo , showqnumcode.

Yes, but in 3.X : they are in Global settings. In 5 and 6 : they are in global survey settings.

In 3.X : superadmin can force the settings to ON , OFF or let admin choose.

@tiborpacalat
Copy link
Collaborator

My comment was only regarding testing this PR.

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 Needs testing
Projects
None yet
4 participants