In fact, manually looking at the settings, they are all copied as value 'I', which is indeed a simple copy.
The non-changeable settings though, seem to require the actual setting which should be set at activation time to the value to be interited?

Feel free to do a pull request.

I would, if I felt anywhere near comfortable to safely make this adjustment in the LS code. I do not.

As it stands, AFAIK, using Remove control, I cannot use a get call to see what the inheritable settings are (doing get_survey_properties on the survey returns 'inherit', and the settings are not in the get_global settings).
So with remote control I can't get further than to manually set the survey settings prior to activation, to settings that are fixed and not related to the actual configured settings to be inherited.

Without that workaround, activating a survey using Remote Control breaks the activated survey and my intend was to inform you of this. It is not intended to suggest anyone is obliged to fix this, and if I could do it myself (without surely breaking functionality or stability of the code), I would. Sorry about that.

This is there even in LS 2.05+.
In any one survey, stop survey, enable both Datestamp and timeings save, start survey, stop survey, disable timings but keep datestamp enabled, start survey, remove the old tables - survey and timings, now all should be well.
Had to also initialise the array as in:

        if($sType=="hour")
            $dFormat = "Y-m-d_G";
        else
            $dFormat = "Y-m-d";

        $aRes = array();

in the file application/models/SurveyDynamic.php

Ref commit:
https://github.com/LimeSurvey/LimeSurvey/commit/0eb2bb82e57b245ddc8fdf1364b5bb6b10b47ade#diff-87edfe668680c1cd23e3b07c0697d850d457a144360ff626c64ca383a159faab

@Selcal This is a problem on

• the survey settings being saved?
• the behaviour of the survey after activation?
• the UI which shows NO when it should be something else?

Does this happen as well when activating through regular UI?

@gabrieljenik
The problem is the behaviour of the survey after activation; as it does not take Notification and Data management settings into account. The values are set to I(nherit) which is not possible on an active survey. LS treats this as NO it seems.

Doing the same through the UI, the UI asks specifically for these settings during the activation steps. However, the API call does not take any arguments, so these are not quite equal behaviour.
Logically (my logic!) I would expect either required arguments for the call, to set the values. Or for the activation to read the values to inheret and then set that value.

Either way, setting the database on activated survey to (I)nherit is not correct behaviour as this setting is not valid for an activated survey.

Thanks. Thngs are a bit clearer now.
Have you tried doing "set_survey_properties" before running the activate?

When you say...

doing get_survey_properties on the survey returns 'inherit', and the settings are not in the get_global settings

What do you mean they are no in the get_global settings ?

Thanks

Sorry, I meant get_global_settings. In other words, it's not possible for me to find out dynamically what the properties that are set to Inherit are supposed to be.
In other words, I can't retrieve the 'parent' settings to inherit, and so can only activate the survey using predetermined settings in the script calling the API.

For these settings, the set_survey_properties can only be called prior to activation. After, the settings are restricted (which is what permanently breaks the activated survey).

So at the moment, the settings have to be set before activation, using predetermined values set by the function call. Whereas values set to Inherit would normally... well inherit the settings from the parent :).

For these settings, the set_survey_properties can only be called prior to activation.
So at the moment, the settings have to be set before activation, using predetermined values set by the function call

So, basically it works on RemoteControl the same way it works as with the manual GUI.
right?

Yes and no. The GUI will ask to confirm the settings, which is an extra step. However, it auto-fills the settings dialog with those inherited from the global settings (ie, it automatically takes inherited settings).
But using RC, I cannot retrieve those settings so cannot do the same. And if not properly set, the function breaks the survey.

-Activating with the GUI will, if leaving everything as-is, create a working survey with the values inherited.
-Activating with RC, will create a broken survey with invalid settings (which can no longer be changed as the survey is now active so they are inhibited).

Maybe the RPC needs to use the activate survey class?

The SurveyActivator? Already is

OK, so there's more logic in the controller class then that's not replicated in the RPC class?

OK, so there's more logic in the controller class then that's not replicated in the RPC class?

I thought so at the very begining, but not exactly.

• When doing it manually, the survey settings in question are confirmed through the UI and then saved by the controller.
• The RC don't allow those as parameters.
• When doing through RC the current way to confirm that is by setting the survey properties.
  But if those are "I" and we want to overwrite them, there is no way to see what is the value to be inhertied and to be set.

Activated survey has all notification and data management set to No (unchangeable).

OK, so just add more arguments to the RPC call? Not possible?

That

• or/and make a new method for getting the global_settings when they are I on the suvey.

@Selcal are you up for making a PR with the needed change?

[quote]or/and make a new method for getting the global_settings when they are I on the suvey[/quote]

I think that'd be the way to go since going the arguments route, the RC caller still needs to know the to-be-inherited values for the fields set to I. And right now this isn't possible it seems.
Possibly combined with the activate call doing this automatically when activating.

@gabrieljenik, I can try, but it's going take a long time for me. So far I have some feel with Limesurvey as a user, but haven't gone into the code. It'll take me awhile to figure out what is where and what is safe to change.
Not sure I'll be succesful tbh, simply because I don't that much time to devote to reading into the code. That said, no fair asking anyone to do it if not willing to try myself. No timeline though :).

@tibor.pacalat Let you review if this is something I should tackle or not. Thanks

@gabrieljenik Sure, but only if there are no more pressing critical/blocker issues that are still untouched.

OK. Just assign it when you feel it is the right time. Thanks

I'm picking this up

been doing some testing with this. I would suggest not to use the activate_survey RPC call at all until a fix is in place.

if any of the activation opts are set on the global survey/survey group, and you activate via RPC call, limesurvey does not actually record any survey responses, although you can take the survey just fine. (so there would be data loss.)

so beyond the issue of not being able to set the activation options in the activate_survey call,

limesurvey expects these values to already be set to the inherited value from the global survey/survey group upon activation and does not handle the case where they are still set to "I"

a workaround for now would be to set the survey properties manually before activation (there is no way I can see to get the value you would inherit from the survey group/global survey group, so you would need to decide on the value,)

we should also consider adding the ability to get/set the global survey/survey group settings

master pr: https://github.com/LimeSurvey/LimeSurvey/pull/3595

after review, I can backport to 5.x

basically, the approach is to first use the survey settings that result from merging the survey settings, survey group settings, and global survey settings.

then I also allow for user to pass in activation settings values to override the merged settings (analogous to the settings in the activation dialog)

Why not use directly https://github.com/LimeSurvey/LimeSurvey/blob/ea1c4ba3b12dd817bc820c6239b72e4d476ccd7d/application/models/Survey.php#L163

Survey->oOtions->anonymized

I think it's the way to do.

See for example https://github.com/LimeSurvey/LimeSurvey/blob/ea1c4ba3b12dd817bc820c6239b72e4d476ccd7d/application/core/LimeMailer.php#L284

I don't see usage of datestamp in activator

We have only issue with datestamp ? I check for

basically, the approach is to first use the survey settings that result from merging the survey settings, survey group settings, and global survey settings.

Here the result is already in oOptions, and the usage MUST be the dedicated option : https://github.com/LimeSurvey/LimeSurvey/blob/master/application/models/Survey.php#L114

I will update the pr to use the oOptions property rather than using the getSurveyInfo from common helper to access the merged settings.

I reiterate here that the survey taking part of limesurvey does not handle a value of "I" for the survey activation related settings properly. This results in data loss. I think this problem should also be fixed. But the problem in this particular issue is that the rpc function to activate a survey does not provide equivalent functionality to the admin activate button/dialog (using merged settings, with the option for the user to override), and thus cannot be used to properly activate a survey. my pr addresses this.

we could open a separate issue for the handling of survey related activation settings being set to "I" (inherit) by the survey taking portion of the software.

Some comments, the current PR fix the issue.

From my more user/reporter perspective; it seems that any active survey should not be able to have "I" for survey settings, which is why the survey taking does not handle it properly. At this moment the "I" setting could only exist through the RPC call which this report focusses on. As the admin interface otherwise handles this and sets the value, and no other way of having "I" setting left exists.

If I'm correct, then the survey taking does not need to handle "I", as this is 'simply' an invalid setting. The PR would fix this; "I" would no longer be possible.
For the survey taking part, perhaps it would make more sense if it could handle any invalid setting better? Not limited to "I" but anything thats not correct. So that it doesn't take responses if it won't save them. That'd be a different issue.

@DenisChenu updated pr in response to your feedback, thank you

@Selcal correct, this pr would allow the rpc activate_survey call to replace "I" values with "Y" or "N" based on inheritance from the survey settings, survey group settings, and global survey settings. It also adds the ability for the user to override the inherited value if desired. This results in no "I" values on the activated survey and creates parity between the rpc function and the activation button/dialog in the admin interface.

I also agree that the software should properly handle an "I" or invalid setting in an activated survey. Maybe default to inherited value? Because at present this case causes data loss.

I also agree that the software should properly handle an "I" or invalid setting in an activated survey. Maybe default to inherited value? Because at present this case causes data loss.

Also agree (and i start a comment about this on PR) BUT
The issue here is about LSA import : LSA contains response table. Maybe import LSA lss with inherited value for this settings ?

not related at all to LSA import

not related at all to LSA import

If you leave I in survey, when export it set to I
If I result before export is N, and after import is Y : you broke the LSA.

It's totally related to LSA here.

Patch is OK for the purpose (same than GUI)

PHP7 speed difference between isset and array_key_exist is low.

Ready for testing

this issue relates to survey activation using RPC function activate_survey only

why related to LSA import???
For example, using field ipaddr,
current behavior, export with ipaddr = "I", import file, in new survey ipaddr is still set to "I"
Do you mean if inherited value is "N" and current value is "I", LSA should export "N" and then import "N"? Because user might change the value that would be inherited to "Y" in the time between export and import? And that would mean LSA export "I" and then import "Y"? And this breaks LSA?

I'm confused what you mean

the settings are only changed when activating the survey

export with "I" -> import with "I"
export with "Y" -> import with "Y"
export with "N" -> import with "N"

after activating, they can be only "Y" or "N". "I" is not possible. so then
export with "Y" -> import with "Y"
export with "N" -> import with "N"

think its okay

Do you mean if inherited value is "N" and current value is "I", LSA should export "N" and then import "N"? Because user might change the value that would be inherited to "Y" in the time between export and import? And that would mean LSA export "I" and then import "Y"? And this breaks LSA?

Yes,

after activating, they can be only "Y" or "N". "I" is not possible. so then

Then: it's OK :)

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=35722

here's the pr for 5.x: https://github.com/LimeSurvey/LimeSurvey/pull/3609

I tested the fix for 5.x, works! @DenisChenu can you please do the core review?

@tibor.pacalat its the same commit from master cherry picked to 5.x so exact same code

I just check SonarCloud to be green and merge it.

Fix committed to 5.x branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=35744

Fixed in Release 6.3.5+231113