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

[Core] Add config option for projects to specify how they want dates to be displayed #5004

Merged
merged 2 commits into from
Feb 27, 2020
Merged

[Core] Add config option for projects to specify how they want dates to be displayed #5004

merged 2 commits into from
Feb 27, 2020

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Aug 6, 2019

Brief summary of changes

Following today's meeting, this adds a config option to allow projects to specify how they want dates to be displayed in LORIS.

This PR only adds two quick examples of using this Config setting; it is by no means comprehensive.

Also, this will only take place in "PHP contexts" and not within JS date elements as these are OS- and locale-dependent.

Testing

  1. On aces/major go to the issue tracker module and look at the Last Updated column.
  2. Checkout my branch and run the SQL patch.
  3. Go to the configuration module and change the date format to something like ymd.
  4. Reload the issue tracker module. The date format in the Last Updated column should match the format you supplied in step 3.

@johnsaigle johnsaigle added Category: Feature PR or issue that aims to introduce a new feature Area: UI PR or issue related to the user interface [branch] major labels Aug 6, 2019
@johnsaigle johnsaigle requested a review from ridz1208 August 6, 2019 17:02
@@ -252,3 +253,4 @@ INSERT INTO Config (ConfigID, Value) SELECT ID, 't1' FROM ConfigSettings WHER
INSERT INTO Config (ConfigID, Value) SELECT ID, 't2' FROM ConfigSettings WHERE Name="modalities_to_deface";
INSERT INTO Config (ConfigID, Value) SELECT ID, 'pd' FROM ConfigSettings WHERE Name="modalities_to_deface";
INSERT INTO Config (ConfigID, Value) SELECT ID, 'false' FROM ConfigSettings WHERE Name="usePwnedPasswordsAPI";
INSERT INTO Config (ConfigID, Value) SELECT ID, 'Y-m-d' FROM ConfigSettings WHERE Name="dateDisplayFormat";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default value here is the ISO date standard.

@johnsaigle johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Aug 6, 2019
@johnsaigle
Copy link
Contributor Author

Need to add Raisinbread changes.

@johnsaigle
Copy link
Contributor Author

I'm getting the error:

  1. NextStageTestIntegrationTest::testNextStageDoespageLoad
    Failed asserting that 'Fatal error: Uncaught ConfigurationException: Config setting dateDisplayFormat does not exist in database in /app/php/libraries/NDB_Config.class.inc:302

I don't know why it says the config setting doesn't exist... the patch looks right to me and worked on my VM. Am I doing something wrong that makes it so Travis can't see this config setting?

@johnsaigle johnsaigle added "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help Language: SQL PR or issue that update SQL code labels Aug 12, 2019
@ridz1208
Copy link
Collaborator

@johnsaigle it's becaude the RB changes have not been done. the RB data replaces the defualt configuration to mimic a real project sourcing the patches

@johnsaigle
Copy link
Contributor Author

@ridz1208 Thanks for clarifying. I'll fix this

@johnsaigle johnsaigle removed "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help State: Needs work PR awaiting additional work by the author to proceed labels Aug 12, 2019
PapillonMcGill
PapillonMcGill previously approved these changes Aug 26, 2019
@johnsaigle johnsaigle added the State: Needs documentation PR that needs a more exhaustive documentation to proceed label Sep 9, 2019
@johnsaigle johnsaigle removed the State: Needs documentation PR that needs a more exhaustive documentation to proceed label Sep 10, 2019
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

I think what we want here is to use some kind of php date functionality that grabs dates in sql format (yyyy-mm-dd) and converts it to the desired format using the dateDisplayFormat config. i.e. php's date() function except this function returns the current timestamp. we need an accompanying piece that does something useful with the default 'Y-m-d'

raisinbread/RB_files/RB_ConfigSettings.sql Outdated Show resolved Hide resolved
raisinbread/RB_files/RB_Config.sql Outdated Show resolved Hide resolved
modules/user_accounts/php/edit_user.class.inc Outdated Show resolved Hide resolved
@johnsaigle
Copy link
Contributor Author

@zaliqarosli You're right, the examples I used didn't actually make sense.

I went into issue tracker like you suggested and changed the dates that way. Rather than pass an unformatted date into JS and format it there, I created a function to convert the date format in PHP before it gets passed into the front-end.

I chose to do this because the config setting is a PHP date format such as 'Y-m-d' and there's no native way to convert that format into something that JS understands.

I updated the PR description with testing instructions.

@zaliqarosli zaliqarosli added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jan 6, 2020
@johnsaigle johnsaigle added State: Needs RB update PR that needs to update Raisinbread to reflect its changes and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Jan 28, 2020
@johnsaigle johnsaigle removed the State: Needs RB update PR that needs to update Raisinbread to reflect its changes label Feb 10, 2020
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

rebasing not quite successful ... changes to edit_user.class.inc not necessary - issue tracker changes should suffice!

'language' => 'en',
'format' => 'YMd',
'format' => $dateFormat,
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe this date option is for a date input element. this PR wants to format date displays i.e. date shown as string

Comment on lines 95 to 98
INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (99,65,'true');
INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (100,19,'false');
INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (101,65,'true');
INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (102,101,'Y-m-d H:i:s');
Copy link
Contributor

Choose a reason for hiding this comment

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

these changes are unintentionally affecting other configs

INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (103,102,'/data/document_repository_uploads/');
INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (104,103,'/data/data_release_uploads/');
INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (105,104,'Y-m-d H:i:s');
Copy link
Contributor

Choose a reason for hiding this comment

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

you have this config duplicated on line 98

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll redo the RB export

@johnsaigle johnsaigle added the State: Needs RB update PR that needs to update Raisinbread to reflect its changes label Feb 10, 2020
@ridz1208 ridz1208 requested a review from zaliqarosli February 12, 2020 12:48
@ridz1208 ridz1208 removed the State: Needs RB update PR that needs to update Raisinbread to reflect its changes label Feb 12, 2020
@ridz1208
Copy link
Collaborator

@johnsaigle from what I can see, in RB you are adding the config setting but not the actual config value, which means the default will be used in the issue tracker which is ATOM

is that your intent ?

@johnsaigle
Copy link
Contributor Author

No I think I need to fix that. I was having some issues with raisinbread exporting + rebasing

@ridz1208
Copy link
Collaborator

@johnsaigle let me know if you need help with it

@ridz1208 ridz1208 self-assigned this Feb 13, 2020
@johnsaigle
Copy link
Contributor Author

Feel free to do it. I think you should be abel to push to my branch. Let me know if there's an issue

@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 24, 2020
johnsaigle and others added 2 commits February 26, 2020 20:56
Update RB files

Improve wording of SQL description text

Apply suggestions from code review

Revert URL and host changes

Fix Raisinbread files

Add issue tracker example and Utility function

Apply suggestions from code review (remove bad config data)

Update raisinbread/RB_files/RB_Config.sql

remove old example

Update raisinbread/RB_files/RB_Config.sql

Revert another example

fix typo

Change defauly format to include hours, minutes, and seconds

Update Raisinbread files after rebase

update raisinbread

change rb again

add default formt in utility function

remove extra lines from rebase

Redo RB changes

Revert changes to RB admin user

remove bad code from rebase

revert changes to edit user
@ridz1208 ridz1208 added Passed manual tests PR has been successfully tested by at least one peer and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Feb 27, 2020
@ridz1208
Copy link
Collaborator

@driusan all urs

@driusan driusan merged commit 6fdce2b into aces:master Feb 27, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Feb 27, 2020
@ridz1208 ridz1208 added the Release: Add to release notes PR whose changes should be highlighted in the release notes label Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI PR or issue related to the user interface Category: Feature PR or issue that aims to introduce a new feature Language: SQL PR or issue that update SQL code Passed manual tests PR has been successfully tested by at least one peer Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants