View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
18381 | Feature requests | Plugins | public | 2022-09-28 11:55 | 2023-01-30 10:22 |
Reporter | bismark | Assigned To | ollehar | ||
Priority | normal | Severity | feature | ||
Status | ready for code review | Resolution | open | ||
Summary | 18381: PluginSetting type date must be saved as a common datetime string | ||||
Description | my personal dateformat is | ||||
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) | ||||
Tags | No tags attached. | ||||
Bug heat | 18 | ||||
child of | 18330 | closed | DenisChenu | Bug reports | SurveySetting type date doesnt show datepicker |
Can you fix this without breaking old plugins? |
|
I don't think this is a bug, but a feature. |
|
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. |
|
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. 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. |
|
We have same issue with password : how to fix it …
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
Maybe best solution is to force datetime to be in ISO/SQL format here ? |
|
Forcing ISO/SQL format is fine with me. We just need to make sure that users are not able to enter something invalid. |
|
@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 ? |
|
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? |
|
@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 : |
|
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)? |
|
isodate is better than saveasiso :+1: Currently not a lot of time :) |
|
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? |
|
Extra options |
|
HI @DenisChenu, From what I spoken with Marcel, you will take this, right? |
|
? I don't remind , but OK :) |
|
@DenisChenu, is there a certain timeline for this? |
|
Oups , was on feature … |
|
Shit WhDateTimePicker didn't allow adding function in https://bootstrap-datepicker.readthedocs.io/en/latest/events.html# |
|
If this would have been easy to implement, we would not have asked you to look into it ;-) |
|
Is that a frontend based solution? |
|
Seems WhDateTimePicker is out of date … no way to get js events working … |
|
https://github.com/LimeSurvey/LimeSurvey/pull/2753 Update plugin |
|
@bismark, I think this implementation makes sense and offer sufficient flexibility to adjust our plugins as needed? |
|
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 … |
|
All plugin settings are frontend helper only : currently NOTHING are checked or updated by default. See the commit, i need to add hidden in frontend to check it in backend. We can check setting type for Global Setting Plugin must/can do own validation in NewSurveySettings event |
|
Why not? |
|
We don't have any information on settings set in beforeSurveySettings |
|
PS : adding beforeSurveySettings a second time in Database : you surely broke some plugins … |
|
Converting date here wouldn't work? |
|
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 We keep old method and create a new one 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. |
|
If not, what you did on DB is OK. When saying this, ...
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 Can that be moved to a new getForSurvey() method? Maybe that's a bit cleaner ? |
|
The only way to get PHP information on surveySettings are to
Why ? The plugin read with $this->get('setting','survey',$sid) , it's the plugin task to don't break anything with this …
Then add again exception ? And after have all getForUser etc … ? I don't like this … |
|
@gabrieljenik about «All plugin settings are frontend helper only» you don't seem to trust me on this point? Have you looked, tried? If you don't trust me on this point: take the bug and create an alternative PR, it will go faster. |
|
It is not a matter about trust. I believe there could be a different approach which I like more. If your approach works and my arguments don't convince you, then, good, keep on going. Just please also invite Olle or Casten to do a review as their point of view will probably be helpfull. |
|
I really want a clean solution to have a real PHP control too. Now : there are 2 solution : rewriting WHOLE plugin setting process to allow a real PHP control You currently discuss on « rewriting WHOLE plugin setting»… |
|
PS : like i write in commit : https://github.com/LimeSurvey/LimeSurvey/pull/2753#issuecomment-1333844087
I mean create a new function in PluginBase like But since there are 2 commit currently needing it … i need to have one merged … |
|
I don't think I am saying rewrite whole plugin system. So, I will just review the code and move over :) |
|
Tell me clearly the different approach ? Without rewriting PluginSetting system … There are no back-end solution with the current Plugin setting system, i write it and try to explain … but you ask again why … |
|
Yes, we are trying to get in sync, but can't get to it. Still, as to try one more time, basically my idea is:
But that's an idea. I haven't done a PoC. Thanks! |
|
@ollehar, can we merge this into the next release? |
|
@ollehar, just a friendly reminder... |
|
Hm. Small conflict in the PR now. Wrote a comment. |
|
Done |
|
Here : |
|
@bismark, I think this works as needed? @DeniChenu, thanks for your help with this. |
|
I think it's better to save as Y-m-d H:i:s by default Denis |
|
I downloaded TestPlugin and tried to save global plugin settings, which results in: |
|
Oups … sorry for this … |
|
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 | |
2022-12-05 08:11 | DenisChenu | Note Added: 72991 | |
2022-12-05 08:15 | DenisChenu | Note Added: 72992 | |
2022-12-05 13:31 | gabrieljenik | Note Added: 73002 | |
2022-12-05 15:21 | DenisChenu | Note Added: 73005 | |
2022-12-05 15:21 | DenisChenu | Assigned To | => ollehar |
2022-12-05 15:28 | DenisChenu | Note Added: 73006 | |
2022-12-05 16:23 | gabrieljenik | Note Added: 73007 | |
2022-12-05 16:31 | DenisChenu | Note Added: 73008 | |
2022-12-05 16:37 | gabrieljenik | Note Added: 73009 | |
2022-12-08 16:52 | Mazi | Note Added: 73042 | |
2022-12-19 13:55 | Mazi | Note Added: 73182 | |
2022-12-19 16:58 | ollehar | Note Added: 73188 | |
2022-12-19 19:20 | DenisChenu | Note Added: 73193 | |
2022-12-19 19:20 | DenisChenu | File Added: TestDateSetting-3.zip | |
2022-12-19 19:22 | DenisChenu | Note Added: 73194 | |
2022-12-19 19:22 | DenisChenu | Note Edited: 73194 | |
2022-12-20 09:56 | Mazi | Note Added: 73200 | |
2022-12-20 09:57 | DenisChenu | Note Added: 73201 | |
2023-01-30 10:01 | bismark | Note Added: 73596 | |
2023-01-30 10:01 | bismark | Bug heat | 16 => 18 |
2023-01-30 10:21 | DenisChenu | Note Added: 73597 |