Relationship Graph

Relationship Graph
related to related to child of child of duplicate of duplicate of

View Issue Details

This bug affects 2 person(s).
 16
IDProjectCategoryView StatusLast Update
18381Feature requestsPluginspublic2022-12-02 20:41
Reporterbismark Assigned To 
PrioritynormalSeverityfeature 
Status ready for code reviewResolutionopen 
Summary18381: PluginSetting type date must be saved as a common datetime string
Description

my personal dateformat is tt.mm.jjjj
now a PluginSetting type date will be saved as 02.09.2022 15:30
but I would expect a common date format like 2022-09-02 15:30

Steps To Reproduce

Steps to reproduce

(Replace this text with detailed step-by-step instructions on how to reproduce the issue)

Expected result

(Write here what you expected to happen)

Actual result

(Write here what happened instead)

TagsNo tags attached.
Bug heat16

Relationships

child of 18330 closedDenisChenu Bug reports SurveySetting type date doesnt show datepicker 

Activities

ollehar

ollehar

2022-09-28 13:19

administrator   ~71995

Can you fix this without breaking old plugins?

c_schmitz

c_schmitz

2022-09-28 15:35

administrator   ~72005

I don't think this is a bug, but a feature.
Currently there is no typing at all used for saving plugin settings. The existing typing only determines how the setting is displayed using the SettingsWidget.

c_schmitz

c_schmitz

2022-09-28 15:39

administrator   ~72006

To keep backward_compatibilty I would just introduce a new field type and name it 'datetime' instead of 'date'. Mark 'date' as obsolete for future major versions.

Mazi

Mazi

2022-09-30 10:07

updater   ~72025

The fix for https://bugs.limesurvey.org/view.php?id=18330 leads to a far better usage of date type fields at plugin settings. But it can also cause serious issues. A user with date format setting yyyy-mm-dd may be entering a plugin setting like 2022-12-31. The date may be used for certain calculations or triggering email sending.
Then another user e.g. from Germany with date format dd.mm.yyyy edits the same setting and changes the day to "24" but then it gets saved as 24.12.2022 at the DB. All calculations or the like will be broken.

So whatever solution we aim for, we should make sure that we always have a valid date format stored for this field type, similar to how we deal with a token's expiry date for example.

DenisChenu

DenisChenu

2022-09-30 12:24

developer   ~72028

Can you fix this without breaking old plugins?

We have same issue with password : how to fix it …

To keep backward_compatibilty I would just introduce a new field type and name it 'datetime' instead of 'date'. Mark 'date' as obsolete for future major versions.

We don' have different way to save settings according to type currently … we can fix it for surveySetting before send it to the event. But for global one : there are only one function maybe extended by plugin.

Survey : https://github.com/LimeSurvey/LimeSurvey/blob/32aa344f92cee033805f3f788aac2f48a8cfded0/application/controllers/admin/Database.php#L661 We can update some before send it to plugin
Global : https://github.com/LimeSurvey/LimeSurvey/blob/32aa344f92cee033805f3f788aac2f48a8cfded0/application/libraries/PluginManager/PluginBase.php#L257 (i extend it a lot …)

The fix for https://bugs.limesurvey.org/view.php?id=18330 leads to a far better usage of date type fields at plugin settings.

Maybe best solution is to force datetime to be in ISO/SQL format here ?

Mazi

Mazi

2022-10-04 12:10

updater   ~72068

Forcing ISO/SQL format is fine with me. We just need to make sure that users are not able to enter something invalid.

Mazi

Mazi

2022-10-28 14:25

updater   ~72463

@c_schmitz, can we assign this to a developer so it gets included at the next sprint? We need it in 2-4 weeks for a new project.

@DenisChenu, do you want to pick this one since you also worked on https://bugs.limesurvey.org/view.php?id=18330 ?

c_schmitz

c_schmitz

2022-10-28 15:07

administrator   ~72464

I am sorry but it is not a priority for us and it is more like a new feature. Maybe you can hire Denis directly, if this can be implemented in a backward-comptible manner?

DenisChenu

DenisChenu

2022-10-28 15:23

developer   ~72465

@bismark have competency to do it i think :)

Else : the way i do it for password ? https://github.com/LimeSurvey/LimeSurvey/pull/2651

Adding a option : 'saveasiso'=>true

Mazi

Mazi

2022-10-28 15:40

updater   ~72466

My preference is adding a new setting type "isodate" which has a date picker but always stores details as YYYY-MM-DD format.

We just need the time for implementing this. @gabrieljenik, would you be able to provide a helping hand (paid support)?

DenisChenu

DenisChenu

2022-10-28 15:41

developer   ~72467

isodate is better than saveasiso :+1:

Currently not a lot of time :)

gabrieljenik

gabrieljenik

2022-10-31 14:35

manager   ~72486

I think the term "attribute on the setting type" and "setting type" are begin used inter-changabely.

With an attribute on the setting type, the impact is minor I think, right?

DenisChenu

DenisChenu

2022-10-31 15:31

developer   ~72490

With an attribute on the setting type, the impact is minor I think, right?

Extra options 'isodate' => false by default don't change anything for old plugins.

gabrieljenik

gabrieljenik

2022-11-23 14:29

manager   ~72860

HI @DenisChenu,

From what I spoken with Marcel, you will take this, right?

DenisChenu

DenisChenu

2022-11-23 14:41

developer   ~72861

?

I don't remind , but OK :)

Mazi

Mazi

2022-12-01 09:55

updater   ~72953

@DenisChenu, is there a certain timeline for this?

DenisChenu

DenisChenu

2022-12-01 11:05

developer   ~72957

Oups , was on feature …

TestDateSetting.zip (1,352 bytes)
DenisChenu

DenisChenu

2022-12-01 12:42

developer   ~72958

Shit WhDateTimePicker didn't allow adding function in https://bootstrap-datepicker.readthedocs.io/en/latest/events.html#

Mazi

Mazi

2022-12-01 13:04

updater   ~72959

If this would have been easy to implement, we would not have asked you to look into it ;-)

gabrieljenik

gabrieljenik

2022-12-01 13:30

manager   ~72960

Shit WhDateTimePicker didn't allow adding function

Is that a frontend based solution?
I thoguht it would be more inclined into a backend handling.

DenisChenu

DenisChenu

2022-12-01 13:41

developer   ~72961

Seems WhDateTimePicker is out of date … no way to get js events working …

DenisChenu

DenisChenu

2022-12-01 14:43

developer   ~72963

https://github.com/LimeSurvey/LimeSurvey/pull/2753

Update plugin

        'checkDate1'=>array(
            'type'=>'date',
            'label' => 'A date, only year and month was saved',
            'default' => '',
            'saveformat' => "Y-m"
        ),
        'checkDate2'=>array(
            'type'=>'date',
            'label' => 'Another date',
            'default' => '',
        ),
        'checkDate3'=>array(
            'type'=>'date',
            'label' => 'A date saved as shown',
            'default' => '',
            'saveformat' => null
        ),
TestDateSetting-2.zip (1,431 bytes)
Mazi

Mazi

2022-12-01 15:00

updater   ~72965

@bismark, I think this implementation makes sense and offer sufficient flexibility to adjust our plugins as needed?

DenisChenu

DenisChenu

2022-12-01 15:11

developer   ~72966

I'm really unhappy to don't find a JS way only …

I try all events in https://bootstrap-datepicker.readthedocs.io/en/latest/events.html

No one seems to work …

DenisChenu

DenisChenu

2022-12-01 15:16

developer   ~72967

@gabrieljenik

Is that a frontend based solution?

All plugin settings are frontend helper only : currently NOTHING are checked or updated by default.
We can not have a "only backend" solution …

See the commit, i need to add hidden in frontend to check it in backend.

We can check setting type for Global Setting
But for NewSurveySettings : we can not add an event BeforeSurveySettings just to have current SurveySettings information.
Then : we have only frontend validation for core.

Plugin must/can do own validation in NewSurveySettings event

gabrieljenik

gabrieljenik

2022-12-01 17:19

manager   ~72973

We can not have a "only backend" solution …

Why not?

DenisChenu

DenisChenu

2022-12-01 17:21

developer   ~72974

We don't have any information on settings set in beforeSurveySettings
https://github.com/LimeSurvey/LimeSurvey/blob/e54d2f532b363e474f679e6392e59da06efb8000/plugins/Demo/Example/Example.php#L30

DenisChenu

DenisChenu

2022-12-01 17:22

developer   ~72975

PS : adding beforeSurveySettings a second time in Database : you surely broke some plugins …

gabrieljenik

gabrieljenik

2022-12-01 17:51

manager   ~72976

Converting date here wouldn't work?
https://github.com/LimeSurvey/LimeSurvey/blob/master/application/libraries/PluginManager/PluginBase.php#L275

DenisChenu

DenisChenu

2022-12-01 18:01

developer   ~72978

Last edited: 2022-12-01 18:02

Oups,
Not https://github.com/LimeSurvey/LimeSurvey/blob/e54d2f532b363e474f679e6392e59da06efb8000/application/libraries/PluginManager/PluginBase.php#L257 but

function set : issue it's same in function set : you don't have information on Survey settings …

gabrieljenik

gabrieljenik

2022-12-02 20:32

manager   ~72987

function set : issue it's same in function set : you don't have information on Survey settings …

Well, then maybe we should create a new method setForSurvey()?

Current set method
protected function set($key, $data, $model = null, $id = null)

We keep old method and create a new one
protected function setForSurvey($key, $data, $surveyId = null)

Signature would be pretty similar, but when using that we would be already sure that the id parameter (4th one in the signature) belongs to a survey id.

gabrieljenik

gabrieljenik

2022-12-02 20:41

manager   ~72988

If not, what you did on DB is OK.

When saying this, ...

Is that a frontend based solution?

The conversation got driven to the save process.

I realize we should be reviewing the read process.

Almost all these you did on the SettingsWidget
https://github.com/LimeSurvey/LimeSurvey/pull/2753/files#diff-6bbdf0ee6329bf62fdeb4326696b98f1a3b4879e075f9a9004a37e0d533ec34cR621

Can that be moved to a new getForSurvey() method?
protected function getForSurvey($key, $surveyId = null)

Maybe that's a bit cleaner ?

Issue History

Date Modified Username Field Change
2022-09-28 11:55 bismark New Issue
2022-09-28 13:19 ollehar Note Added: 71995
2022-09-28 13:19 ollehar Bug heat 0 => 2
2022-09-28 15:28 gabrieljenik Status new => acknowledged
2022-09-28 15:35 c_schmitz Note Added: 72005
2022-09-28 15:35 c_schmitz Bug heat 2 => 4
2022-09-28 15:39 c_schmitz Note Added: 72006
2022-09-30 10:07 Mazi Note Added: 72025
2022-09-30 10:07 Mazi Bug heat 4 => 6
2022-09-30 10:07 Mazi Relationship added child of 18330
2022-09-30 12:24 DenisChenu Note Added: 72028
2022-09-30 12:24 DenisChenu Bug heat 6 => 8
2022-10-04 12:10 Mazi Note Added: 72068
2022-10-28 14:25 Mazi Note Added: 72463
2022-10-28 15:07 c_schmitz Note Added: 72464
2022-10-28 15:23 DenisChenu Note Added: 72465
2022-10-28 15:40 Mazi Note Added: 72466
2022-10-28 15:41 DenisChenu Note Added: 72467
2022-10-31 13:50 gabrieljenik Project Bug reports => Feature requests
2022-10-31 14:35 gabrieljenik Note Added: 72486
2022-10-31 14:35 gabrieljenik Bug heat 8 => 10
2022-10-31 14:36 gabrieljenik Assigned To => gabrieljenik
2022-10-31 14:36 gabrieljenik Status acknowledged => assigned
2022-10-31 15:31 DenisChenu Note Added: 72490
2022-11-01 18:34 guest Bug heat 10 => 16
2022-11-23 14:28 gabrieljenik Assigned To gabrieljenik => DenisChenu
2022-11-23 14:29 gabrieljenik Note Added: 72860
2022-11-23 14:41 DenisChenu Note Added: 72861
2022-11-23 14:41 DenisChenu Priority none => normal
2022-11-23 14:41 DenisChenu Severity @50@ => feature
2022-12-01 09:55 Mazi Note Added: 72953
2022-12-01 11:05 DenisChenu Note Added: 72957
2022-12-01 11:05 DenisChenu File Added: TestDateSetting.zip
2022-12-01 12:42 DenisChenu Note Added: 72958
2022-12-01 13:04 Mazi Note Added: 72959
2022-12-01 13:30 gabrieljenik Note Added: 72960
2022-12-01 13:41 DenisChenu Note Added: 72961
2022-12-01 14:43 DenisChenu Note Added: 72963
2022-12-01 14:43 DenisChenu File Added: TestDateSetting-2.zip
2022-12-01 15:00 Mazi Note Added: 72965
2022-12-01 15:11 DenisChenu Note Added: 72966
2022-12-01 15:11 DenisChenu Assigned To DenisChenu =>
2022-12-01 15:11 DenisChenu Status assigned => ready for code review
2022-12-01 15:16 DenisChenu Note Added: 72967
2022-12-01 17:19 gabrieljenik Note Added: 72973
2022-12-01 17:21 DenisChenu Note Added: 72974
2022-12-01 17:22 DenisChenu Note Added: 72975
2022-12-01 17:51 gabrieljenik Note Added: 72976
2022-12-01 18:01 DenisChenu Note Added: 72978
2022-12-01 18:02 DenisChenu Note Edited: 72978
2022-12-02 20:32 gabrieljenik Note Added: 72987
2022-12-02 20:41 gabrieljenik Note Added: 72988