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.

Sign up for GitHub

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

Jump to bottom

Fixes #23671: Locale issue: date picker outputs date in wrong order #5157

Closed
wants to merge 3 commits into from
Closed

Fixes #23671: Locale issue: date picker outputs date in wrong order #5157

wants to merge 3 commits into from

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Nov 3, 2023 •
edited

https://issues.rudder.io/issues/23671
⚠️ We should validate that this change doesn't break anything and need more attention. We should not merge this without validating that is not breaking any other part of Rudder

@ElaadF
Copy link
Member Author

ElaadF commented Nov 3, 2023

The patch is a bit shorter than that....

@ElaadF ElaadF added the WIP Use that label for a Work In Progress PR that must not be merged yet label Nov 3, 2023
@ElaadF
Copy link
Member Author

ElaadF commented Nov 3, 2023

PR rebased

@ElaadF ElaadF force-pushed the bug_23671/locale_issue_date_picker_outputs_date_in_wrong_order branch from 8932a0e to e4ba7fd Compare November 3, 2023 08:57
@amousset
Copy link
Member

amousset commented Nov 3, 2023

This breaks actual tests:

[2023-11-03T09:06:42.147Z] Failed tests:   French Datecomparator  should::accept valid French date(com.normation.rudder.domain.queries.CmdbQueryTest): Invalid parsing: Inconsistency: Invalide date: '23/07/2012', expected format is: 'yyyy/MM/dd'. Error was: Invalid format: "23/07/2012" is malformed at "12"
[2023-11-03T09:06:42.147Z]   French Datecomparator  should::successfully onvert to LDAP a valid French date(com.normation.rudder.domain.queries.CmdbQueryTest): Invalid parsing: Inconsistency: Invalide date: '23/07/2012', expected format is: 'yyyy/MM/dd'. Error was: Invalid format: "23/07/2012" is malformed at "12"

@VinceMacBuche
Copy link
Member

I do think we want to enforce dd/mm/yyyy everywhere (I think it just have to be forced on client side)

VinceMacBuche
VinceMacBuche reviewed Nov 3, 2023
View reviewed changes
...ources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/queries/CmdbQuery.scala Outdated
@@ -564,7 +564,7 @@ case object OrderedStringComparator extends TStringComparator {

case object DateComparator extends LDAPCriterionType {
override val comparators = OrderedComparators.comparators.filterNot(c => c == Regex || c == NotRegex)
val fmt = "dd/MM/yyyy"
val fmt = "yyyy/MM/dd"
Copy link
Member

Choose a reason for hiding this comment

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

I would not change that

Copy link
Member

Choose a reason for hiding this comment

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

ignore my comment

Copy link
Member

Choose a reason for hiding this comment

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

Why not use RFC3339 format ? If we are changing it at all? (ie yyyy-MM-dd)

VinceMacBuche
VinceMacBuche reviewed Nov 3, 2023
View reviewed changes
...ources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/queries/CmdbQuery.scala
@@ -579,6 +579,7 @@ case object DateComparator extends LDAPCriterionType {
// init a jquery datepicker
override def initForm(formId: String): JsCmd = OnLoad(JsRaw("""var init = $.datepicker.regional['en'];
init['showOn'] = 'focus';
init['dateFormat'] = 'yy/mm/dd';
Copy link
Member

Choose a reason for hiding this comment

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

I would change it to dd/MM/yyyy

Copy link
Member

Choose a reason for hiding this comment

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

keep it it's ok

@VinceMacBuche
Copy link
Member

Go for yyyy/mm/dd and correct the tests

@ElaadF
Copy link
Member Author

ElaadF commented Nov 3, 2023

Commit modified

@ElaadF ElaadF force-pushed the bug_23671/locale_issue_date_picker_outputs_date_in_wrong_order branch 2 times, most recently from bc43b85 to 0ef920e Compare November 3, 2023 15:23
@ElaadF
Copy link
Member Author

ElaadF commented Nov 3, 2023

Commit modified

@ElaadF ElaadF force-pushed the bug_23671/locale_issue_date_picker_outputs_date_in_wrong_order branch from 0ef920e to 2228ec0 Compare November 6, 2023 17:23
@ElaadF
fixup! Work in progress
ed91926 
Fixes #23671: Locale issue : Date picker outputs date in wrong order
@ElaadF ElaadF force-pushed the bug_23671/locale_issue_date_picker_outputs_date_in_wrong_order branch from 2228ec0 to ed91926 Compare November 6, 2023 18:06
@ElaadF
Copy link
Member Author

ElaadF commented Nov 6, 2023

PR updated with a new commit

@ElaadF ElaadF removed the WIP Use that label for a Work In Progress PR that must not be merged yet label Nov 6, 2023
fanf
fanf reviewed Nov 9, 2023
View reviewed changes
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

I don't understand if the change are just at the query langage level for new input, or more.

I'm almost sure that change should have to deal somewhere with a compatibility layer so that existing serialized data, for dyn group queries, or other things, are dealt with, or that the query lang test several input format and try to guess which one it should use.

Please, you need to explain how these compat subject are addressed, and you should not do a PR without a comment explaining the architectural impact of the change (or if none, why so)

@ElaadF ElaadF added the WIP Use that label for a Work In Progress PR that must not be merged yet label Nov 9, 2023
@fanf
Copy link
Member

fanf commented Nov 16, 2023

So, to sum-up and be clear:

  • we don't change serialized format. What is in base, or could have been (group definition, ldap, base, whatever) IS the law. We don't change it anywhere but in a (at least) minor release, along with a clear migration and test plan. This is the minimum for such a case
  • so if we change something, we only change the UI part, and adapt the UI <-> persistence part so that the serialised / api format are not changed

ncharles
ncharles reviewed Nov 16, 2023
View reviewed changes
...ources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/queries/CmdbQuery.scala Outdated
@@ -564,7 +564,7 @@ case object OrderedStringComparator extends TStringComparator {

case object DateComparator extends LDAPCriterionType {
override val comparators = OrderedComparators.comparators.filterNot(c => c == Regex || c == NotRegex)
val fmt = "dd/MM/yyyy"
val fmt = "yyyy/MM/dd"
val frenchFmt = DateTimeFormat.forPattern(fmt).withLocale(Locale.FRANCE)
def error(value: String, e: Exception) = Inconsistency(
s"Invalide date: '${value}', expected format is: '${fmt}'. Error was: ${e.getMessage}"
Copy link
Member

Choose a reason for hiding this comment

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

could you fix this typo also ?

@amousset amousset changed the title Fixes #23671: Locale issue : Date picker outputs date in wrong order Fixes #23671: Locale issue: date picker outputs date in wrong order Nov 16, 2023
@ElaadF ElaadF mentioned this pull request Nov 24, 2023
Merged
@ElaadF
Copy link
Member Author

ElaadF commented Nov 24, 2023

I'm closing this PR, it contains some works about what we can do to change the format in the backend, this is the PR that are fixing the issue by only modifying the date picker expected format for the UI : #5201

@ElaadF ElaadF closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanf fanf fanf left review comments

@ncharles ncharles ncharles left review comments

@VinceMacBuche VinceMacBuche VinceMacBuche left review comments

Assignees
No one assigned
Labels
WIP Use that label for a Work In Progress PR that must not be merged yet
Projects
None yet
Milestone
No milestone
5 participants
@ElaadF @amousset @VinceMacBuche @fanf @ncharles