Mark datepicker field as valid only if it's value is an instance of Date #392

romanblanco · 2019-06-10T16:52:16Z

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1692863
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1692980

Steps to Reproduce from BZ ticket:

create a new service dialog with a datepicker field
add the service dialog to the catalog
in the service catalog select the service dialog and click on order
Clear the date from the field and enter "test" in the datepicker field

JPrause · 2019-06-13T12:56:50Z

@romanblanco can you assign a reviewer. We need this for the 5.10.6 build on Monday, June 17

romanblanco · 2019-06-13T13:22:10Z

@himdel could you do the review?

himdel · 2019-06-13T14:25:20Z

Will do :)

himdel · 2019-06-13T15:19:32Z

src/dialog-user/services/dialogData.ts

@@ -171,6 +171,11 @@ export default class DialogDataService {
      }
    }

+    if (field.type === 'DialogFieldDateControl') {
+      validation.isValid = (fieldValue instanceof Date);


I'm worried this will fail for dynamic fields - the server can't send a Date object, so we have to convert, and that's called only from $onInit it seems, not when updating a field

I believe that setupField should take care of converting data to Date object, maybe I just need to include DialogFieldDateTimeControl, anyway I need to verify this works.

ui-components/src/dialog-user/services/dialogData.ts

Lines 38 to 44 in 0f871a0

if (field.type === 'DialogFieldDateTimeControl') {

if (_.isNull(field.values) || _.isUndefined(field.values)) {

field.dateField = field.timeField = new Date();

} else {

field.dateField = field.timeField = new Date(data.values);

}

}

looks like the Dialog User doesn't work well if the value is updated from Automate.

I've applied a simple Automate method (thanks @d-m-u):

new_date = Time.utc(2019, 07, 02) $evm.object['value'] = new_date

I'm not sure if the method is applied correctly, but from what happens I think the problem is in the Dialog User anyway:

I cleared out the field so it's empty -> the Submit button correctly gets disabled

I checked checkbox that should update the dynamic DialogFieldDateControl field

The field is updated with today's date (not utc(2019, 07, 02)) -> the Submit button is still disabled

If I pick the date from calendar manually, the button gets enabled, as expected.

Now I'm using this to log the data to evm.log:

trigger = $evm.root['dialog_trigger'] $evm.log(:info, "======= trigger value #{trigger}") if trigger == "t" new_date = Time.utc(2019, 07, 02) $evm.object['value'] = new_date $evm.log(:info, "======== new date #{$evm.object['value']}") else # "f" $evm.object['value'] = '' $evm.log(:info, "======== clearing up date #{$evm.object['value']}") end

but can't get the log into evm.log nor automate.log, and in setupField I'm getting only undefined, so I might be using the Automate wrong as well.

With f0cb00c I'm getting the values as Date() even after using Automate, only the validation doesn't get run.

I'm not receiving any values into value attribute, so using default_value.

There's a lot of confusion with using values, value or default_value. I'm not sure what exactly is correct, also I think DialogFieldDateTimePicker won't work correctly if it works with value instead of values.

Yeah, it is pretty confusing. On the automate side, you want to set the 'value' key like you're doing above:
$evm.object['value'] = something

However, on the ui-components side, for most of the field types, default_value is the value that is getting passed back from the refresh API call that we should be looking at to determine what to show to the user after a refresh happens. We changed it in 7dcb1f7. For sorted items, values is the key we use since it needs to be a list, and default_value is simply the one that is selected from that list.

On the ui-components side, because of the way datetime controls work, there's special logic for the date and time parts because the default_value comes in as a string (cause it's just a JSON response), and then it gets parsed and separated into a dateField and a timeField since the controls are separate.

romanblanco · 2019-06-14T12:20:35Z

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1702497 (updated in description)

miq-bot · 2019-06-20T13:03:59Z

Checked commits romanblanco/ui-components@1744f67~...306c507 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1692863

romanblanco · 2019-06-24T12:50:13Z

update:

the validation works now, the only issue is if the value comes from automate, the form does not refresh by itself.

After hovering the form, the 'Submit' button gets updated:

If the value is updated manually by the user, changesHappened is called. $doCheck is called if the new value comes from Automate. I've tried to use

this.onUpdate({ dialogFieldName: this.field.name, value: fieldValue });

called by changesHappened, expecting the form would get updated, but still no change. I'm still trying to find what's missing.

romanblanco · 2019-06-24T13:25:27Z

It seems that the issue is not introduced by the changes in this PR. Creating a new issue (#396), removing [WIP] label.

eclarizio · 2019-06-24T19:09:17Z

src/dialog-user/services/dialogData.ts

+      if (_.isNull(field.default_value) || _.isUndefined(field.default_value)) {
+        field.dateField = field.timeField = new Date();
+      } else {
+        field.dateField = field.timeField = new Date(data.default_value);


timeField both here and on line 48 I don't think mean anything for a DialogFieldDateControl, only if the type is a DialogFieldDateTimeControl.

eclarizio · 2019-06-24T19:21:15Z

If the value is updated manually by the user, changesHappened is called. $doCheck is called if the new value comes from Automate. I've tried to use
this.onUpdate({ dialogFieldName: this.field.name, value: fieldValue });
called by changesHappened, expecting the form would get updated, but still no change. I'm still trying to find what's missing.

Actually, I think it's similar if you have a text box with an automate method that changes the regex validation it doesn't update until you focus/unfocus the field. So, I would agree with that being a separate issue and limiting this PR to simply not allowing something like test to be input into a date field.

martinpovolny · 2019-06-27T07:26:51Z

@eclarizio : please, check again.

eclarizio

@romanblanco One small comment, I think it's still good overall, will approve after a response to my comment.

eclarizio · 2019-06-27T15:36:21Z

src/dialog-user/services/dialogData.ts

    if (data.default_value) {
      defaultValue = data.default_value;
    }

+    if (data.type === 'DialogFieldDateControl' || data.type === 'DialogFieldDateTimeControl') {
+      defaultValue = data.dateField ? new Date(data.dateField) : new Date();


I think my only concern here is if data.dateField is going to ever be populated for DialogFieldDateTimeControl, because up on line 46, we are only setting field.dateField for DialogFieldDateControl. Is that a problem?

because up on line 46, we are only setting field.dateField for DialogFieldDateControl

@eclarizio we're setting the data.dateField and data.timeField for DialogFieldDateTimeControl right above the DialogFieldDateControl, on line L38-L44:

38 if (field.type === 'DialogFieldDateTimeControl') { 39 if (_.isNull(field.values) || _.isUndefined(field.values)) { 40 field.dateField = field.timeField = new Date(); 41 } else { 42 field.dateField = field.timeField = new Date(data.values); 43 } 44 }

update: altough, it's possible that we're not using the data.values here correctly, and should be using data.default_value.

update 2: using default_value for DialogFieldDateTimeControl corrected in #398 (I'll rebase the PR after this one is merged)

I'm still confused about this change here (moving defaultValue=... under the other if, in setDefaultValue).

Previously, default_value coming from the server would get used.
Now, only data.dateField will ever be used for date/time values.

So...
a) why not data.timeField also?
b) should we really never use default_value from the server when dealing with date&time?

why not data.timeField also?

the timeField is only related to DialogFieldDateTimeControl, there's no time field in DialogFieldDateControl

should we really never use default_value from the server when dealing with date&time?

we're using the default_value on the line 46 that @eclarizio has mentioned

OK, so default_value is now read only in setupField and we're using dateField only in setDefaultValue. (LGTM, setDefaultValue is only called by setupField.)

But, that still doesn't explain why timeField gets ignored when in a if (data.type === 'DialogFieldDateControl' || data.type === 'DialogFieldDateTimeControl') condition ;).

@himdel you're right. The (data.type === 'DialogFieldDateControl' || data.type === 'DialogFieldDateTimeControl') condition should be split. I'll update the PR.

update:

I believe the timeField does not need to be included in here, as we're only setting a default_value for the field.

Any change of fields is processed in

ui-components/src/dialog-user/components/dialog-user/dialogField.ts

Lines 88 to 103 in 20a84fe

public dateTimeFieldChanged() {

let dateField = this.dialogField.dateField;

let fullYear = dateField.getFullYear();

let month = dateField.getMonth();

let date = dateField.getDate();

if (this.dialogField.timeField === undefined) {

this.dialogField.timeField = new Date();

}

let hours = this.dialogField.timeField.getHours();

let minutes = this.dialogField.timeField.getMinutes();

let fullDate = new Date(fullYear, month, date, hours, minutes);

this.changesHappened([fullDate]);

}

ah, LGTM then, thanks :)

Mark datepicker field as valid only if it's value is an instance of Date (cherry picked from commit ffdd33d) https://bugzilla.redhat.com/show_bug.cgi?id=1702497 https://bugzilla.redhat.com/show_bug.cgi?id=1726388

simaishi · 2019-07-02T18:17:29Z

Hammer backport details:

$ git log -1
commit 0cdc27e97bdfcbda7aa192213bd6fac3f47e1e22
Author: Martin Hradil <[email protected]>
Date:   Tue Jul 2 11:08:05 2019 +0200

    Merge pull request #392 from romanblanco/bz1692863
    
    Mark datepicker field as valid only if it's value is an instance of Date
    
    (cherry picked from commit ffdd33d7ad457b2f6b895c690716b9bfc1b9bc89)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1702497
    https://bugzilla.redhat.com/show_bug.cgi?id=1726388

himdel self-requested a review June 13, 2019 14:25

himdel reviewed Jun 13, 2019

View reviewed changes

romanblanco force-pushed the bz1692863 branch from 3e9e9ef to 1744f67 Compare June 14, 2019 12:18

romanblanco changed the title ~~Mark datepicker field as valid only if it's value is an instance of Date~~ [WIP] Mark datepicker field as valid only if it's value is an instance of Date Jun 17, 2019

miq-bot added the wip label Jun 17, 2019

romanblanco force-pushed the bz1692863 branch 2 times, most recently from ec8f8c6 to 306c507 Compare June 20, 2019 13:01

romanblanco added 4 commits June 21, 2019 09:40

Mark datepicker field as valid only if it's value is an instance of Date

799c809

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1692863

Convert the component value to Date after refresh from Automate

b9bcb0d

Validation for date-related fields

baf9e64

Corrected specs

fc33cf7

romanblanco changed the title ~~[WIP] Mark datepicker field as valid only if it's value is an instance of Date~~ Mark datepicker field as valid only if it's value is an instance of Date Jun 24, 2019

miq-bot removed the wip label Jun 24, 2019

eclarizio reviewed Jun 24, 2019

View reviewed changes

romanblanco force-pushed the bz1692863 branch from 306c507 to 20a84fe Compare June 25, 2019 09:04

Review response: removed unnecessary assign

20a84fe

eclarizio reviewed Jun 27, 2019

View reviewed changes

romanblanco mentioned this pull request Jun 28, 2019

Corrected values for DialogFieldDateTimeControl #398

Closed

eclarizio approved these changes Jun 28, 2019

View reviewed changes

himdel merged commit ffdd33d into ManageIQ:master Jul 2, 2019

himdel self-assigned this Jul 2, 2019

himdel added bug hammer/yes labels Jul 2, 2019

himdel added this to the Sprint 115 Ending Jul 8, 2019 milestone Jul 2, 2019

romanblanco deleted the bz1692863 branch July 2, 2019 11:18

simaishi added hammer/backported and removed hammer/yes labels Jul 2, 2019

himdel added the dialog user label Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark datepicker field as valid only if it's value is an instance of Date #392

Mark datepicker field as valid only if it's value is an instance of Date #392

romanblanco commented Jun 10, 2019 •

edited by simaishi

Loading

JPrause commented Jun 13, 2019

romanblanco commented Jun 13, 2019

himdel commented Jun 13, 2019

himdel Jun 13, 2019

romanblanco Jun 14, 2019

romanblanco Jun 14, 2019 •

edited

Loading

romanblanco Jun 17, 2019 •

edited

Loading

eclarizio Jun 24, 2019

romanblanco commented Jun 14, 2019

miq-bot commented Jun 20, 2019

romanblanco commented Jun 24, 2019

romanblanco commented Jun 24, 2019 •

edited

Loading

eclarizio Jun 24, 2019

eclarizio commented Jun 24, 2019

martinpovolny commented Jun 27, 2019

eclarizio left a comment

eclarizio Jun 27, 2019

romanblanco Jun 28, 2019 •

edited

Loading

himdel Jun 28, 2019 •

edited

Loading

romanblanco Jun 28, 2019

romanblanco Jun 28, 2019

himdel Jul 1, 2019 •

edited

Loading

romanblanco Jul 1, 2019 •

edited

Loading

himdel Jul 2, 2019

simaishi commented Jul 2, 2019

	if (field.type === 'DialogFieldDateTimeControl') {
	if (_.isNull(field.values) \|\| _.isUndefined(field.values)) {
	field.dateField = field.timeField = new Date();
	} else {
	field.dateField = field.timeField = new Date(data.values);
	}
	}

	public dateTimeFieldChanged() {
	let dateField = this.dialogField.dateField;
	let fullYear = dateField.getFullYear();
	let month = dateField.getMonth();
	let date = dateField.getDate();

	if (this.dialogField.timeField === undefined) {
	this.dialogField.timeField = new Date();
	}

	let hours = this.dialogField.timeField.getHours();
	let minutes = this.dialogField.timeField.getMinutes();

	let fullDate = new Date(fullYear, month, date, hours, minutes);
	this.changesHappened([fullDate]);
	}

Mark datepicker field as valid only if it's value is an instance of Date #392

Mark datepicker field as valid only if it's value is an instance of Date #392

Conversation

romanblanco commented Jun 10, 2019 • edited by simaishi Loading

JPrause commented Jun 13, 2019

romanblanco commented Jun 13, 2019

himdel commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanblanco Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

romanblanco Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanblanco commented Jun 14, 2019

miq-bot commented Jun 20, 2019

romanblanco commented Jun 24, 2019

romanblanco commented Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

eclarizio commented Jun 24, 2019

martinpovolny commented Jun 27, 2019

eclarizio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanblanco Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

himdel Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

romanblanco Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simaishi commented Jul 2, 2019

romanblanco commented Jun 10, 2019 •

edited by simaishi

Loading

romanblanco Jun 14, 2019 •

edited

Loading

romanblanco Jun 17, 2019 •

edited

Loading

romanblanco commented Jun 24, 2019 •

edited

Loading

romanblanco Jun 28, 2019 •

edited

Loading

himdel Jun 28, 2019 •

edited

Loading

himdel Jul 1, 2019 •

edited

Loading

romanblanco Jul 1, 2019 •

edited

Loading