User customizable datetime #1986

eric-hansen · 2023-10-15T19:18:09Z

Closes #1945

Adds:

New options under My Profile -> Look & Feel
-> User can specify custom date and time formats, along with timezone different than app default

I tested this with basic auth login, not against LDAP or other options like OIDC. However, if setUserSession is called during the authentication process, the proper fsession index will be set.

This only changes PHP, JS is untouched. I poked around to see if I could make changes for the JS side as well, but didn't feel comfortable enough yet around the code base to do so.

Some diffs are whitespace changes due to VSCode's formatting of things.

CLAassistant · 2023-10-15T19:18:16Z

All committers have signed the CLA.

marcelfolaron · 2023-10-15T23:40:28Z

Thank you for this PR. This is a really good start. I have a few comments and questions regarding code organization but other than that it looks pretty good to me.

eric-hansen · 2023-10-16T14:04:58Z

Thank you for this PR. This is a really good start. I have a few comments and questions regarding code organization but other than that it looks pretty good to me.

Just to confirm are there any comments I need to respond to? I didn't see any while scrolling through the diffs.

marcelfolaron · 2023-10-15T23:24:59Z

app/Core/Controller.php

+         * This also doesn't effect JS date-time any, only PHP calls for date-time, as
+         * they use incompatible formats.
+         */
+        if (isset($_SESSION['userdata']) && isset($_SESSION['userdata']['id'])) {


Why is this needed in the base controller? If anything I would probably move it to the Auth Service where we initialize the session of the user. (Domain/Auth/Services/Auth.php method setUserSession)

Because it gets overridden when the config file is loaded, at least it did during my testing. This allowed the config to still be read and then we update it based on the user settings.

I didn't move this closer to the config parser because that doesn't have access to the user session (for the user ID) nor is the session started yet, so even if I set the DI for SettingsRepo/Service I wouldn't be able to get the proper setting.

Yeah, the issue I have with this here is that the Base controller should not have anything to do with session or user management. it's for controller management and so in a few months I will have forgotten that there is anything related to session in there. For the problem you mentioned please move it to the header controller which has some session calls already: https://github.com/Leantime/leantime/blob/master/app/Domain/Pageparts/Controllers/Header.php

Ah ok yeah makes sense. Alright I'll move it over today.

app/Core/Controller.php

app/Domain/Auth/Services/Auth.php

app/Domain/Users/Controllers/EditOwn.php

marcelfolaron · 2023-10-15T23:34:41Z

app/Domain/Users/Controllers/EditOwn.php

+        {
+            return [
+                'dates' => [
+                    'default' => 'm/d/Y',


The default should be pulled from the language file still. So int this case $this->__("language.dateformat") that way the dateformat is still based on the user language.

Agreed. I was curious how I could do that as that was what I originally wanted. I'll make that change.

marcelfolaron · 2023-10-15T23:34:51Z

app/Domain/Users/Controllers/EditOwn.php

+                    'RFC850' => 'l, d-M-y'
+                ],
+                'times' => [
+                    'default' => 'h:i A',


See above. Take default from language file

marcelfolaron · 2023-10-15T23:38:15Z

composer.json

@@ -56,6 +51,14 @@
        "vedmant/laravel-feed-reader": "^1.6",
        "ext-fileinfo": "*"
    },
+    "optional": {


Why were those moved to optional? We can't move them to optional since we don't know if users are using S3, ldap, oauth until runtime at which point the packages will be needed.
Please revert this change

My thinking in this was to not block an install for users who don't use S3 or LDAP (primarily done for ext-ldap but then it seemed relevant to others as well). Nothing is stopping the user from installing them still.

But if you prefer I revert this then I can do.

Well the problem is that this is not in our build pipeline. So now we would have to make a lot of other changes to ensure our releases, default dev environments and tests still work. Most of our users do not install Leantime from source but using the release package or docker. This change goes a little beyond the scope of this PR. If you could revert this that would be great. We can have a separate discussion around that topic.

Gotcha I'll revert it, sorry about that.

marcelfolaron · 2023-10-17T17:21:40Z

Thank you for this PR. This is a really good start. I have a few comments and questions regarding code organization but other than that it looks pretty good to me.

Just to confirm are there any comments I need to respond to? I didn't see any while scrolling through the diffs.

Sorry I keep forgetting that I have to actually submit the review before the comments are visible...

marcelfolaron · 2023-10-20T14:57:34Z

Hi Eric,

I had merged the PR but unfortunately had to revert it. Couple issue I found:

Would not save the setting. I got an error that "Y-m-d" was not a key in the array. I assume it is because you have an array with values only. They cannot be accessed via key like that.
It didn't seem to apply the new dateformat. Desprite clearing cache and re login only a few date time outputs were changed. None of the due dates, nor other ticket dates were updated. I appears that the javascript datetime format is not set correctly.

eric-hansen · 2023-10-20T17:15:22Z

1. Would not save the setting. I got an error that "Y-m-d" was not a key in the array. I assume it is because you have an array with values only. They cannot be accessed via key like that.

I'll look into this but I didn't experience this issue. Did it happen just with the Y-m-d format or with others as well?

2. It didn't seem to apply the new dateformat. Desprite clearing cache and re login only a few date time outputs were changed. None of the due dates, nor other ticket dates were updated. I appears that the javascript datetime format is not set correctly.

I'm a little confused here.

So dates in the input box/JavaScript use a different format than what the PHP format supports. I can try to keep them in sync but I wasn't sure what file(s) needed to be touched, and am hoping much like the general date-time stuff being in Language.php, the same can be said for JS.

To make things a bit more easier to manage in the future I'd write a "translater" of PHP date-time format to JS, which shouldn't be too difficult. I would just need to be able to pass the formats from backend to frontend.

eric-hansen added 6 commits October 14, 2023 05:56

mark extra feature deps as optional

4a02bc8

support user choosing their TZ, date and time format

b247071

support custom formats for datetime

7558ce7

set the default tz when defined

5739c71

new label strings

d6c54c5

store user timezone setting in session upon login

99f0ff8

eric-hansen requested a review from a team as a code owner October 15, 2023 19:18

eric-hansen requested review from broskees and removed request for a team October 15, 2023 19:18

Merge branch 'master' into user-customizable-datetime

72b0fc4

eric-hansen changed the title ~~#1945 - User customizable datetime~~ 1945 - User customizable datetime Oct 15, 2023

eric-hansen changed the title ~~1945 - User customizable datetime~~ User customizable datetime Oct 15, 2023

Merge branch 'master' into user-customizable-datetime

0fa50e9

Merge branch 'master' into user-customizable-datetime

042f29a

marcelfolaron requested changes Oct 17, 2023

View reviewed changes

eric-hansen added 6 commits October 18, 2023 15:35

move setting the session data for TZ to Header.php

6479282

use date/time formats from configured language

58123a9

revert change to composer.json

6165857

accomendate potentially longer strings

3569dab

support more time-dates

a9c2a89

Merge branch 'master' into user-customizable-datetime

7dd70b4

eric-hansen requested a review from marcelfolaron October 18, 2023 19:39

marcelfolaron approved these changes Oct 20, 2023

View reviewed changes

marcelfolaron merged commit b56c4c1 into Leantime:master Oct 20, 2023
2 of 3 checks passed

eric-hansen deleted the user-customizable-datetime branch October 20, 2023 14:16

marcelfolaron mentioned this pull request Oct 20, 2023

Revert "User customizable datetime" #2016

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User customizable datetime #1986

User customizable datetime #1986

eric-hansen commented Oct 15, 2023 •

edited

CLAassistant commented Oct 15, 2023 •

edited

marcelfolaron commented Oct 15, 2023

eric-hansen commented Oct 16, 2023

marcelfolaron Oct 15, 2023

eric-hansen Oct 17, 2023

marcelfolaron Oct 18, 2023

eric-hansen Oct 18, 2023

marcelfolaron Oct 15, 2023

eric-hansen Oct 17, 2023

marcelfolaron Oct 15, 2023

marcelfolaron Oct 15, 2023

eric-hansen Oct 17, 2023

marcelfolaron Oct 18, 2023 •

edited

eric-hansen Oct 18, 2023

marcelfolaron commented Oct 17, 2023

marcelfolaron commented Oct 20, 2023

eric-hansen commented Oct 20, 2023

User customizable datetime #1986

User customizable datetime #1986

Conversation

eric-hansen commented Oct 15, 2023 • edited

CLAassistant commented Oct 15, 2023 • edited

marcelfolaron commented Oct 15, 2023

eric-hansen commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelfolaron Oct 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelfolaron commented Oct 17, 2023

marcelfolaron commented Oct 20, 2023

eric-hansen commented Oct 20, 2023

eric-hansen commented Oct 15, 2023 •

edited

CLAassistant commented Oct 15, 2023 •

edited

marcelfolaron Oct 18, 2023 •

edited