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

Fix edit cookie shows expires as an Invalid Date #4897

Merged
merged 4 commits into from
Jul 4, 2022

Conversation

tangweikun
Copy link
Contributor

@tangweikun tangweikun commented Jun 24, 2022

#4195 Fix Edit Cookie shows Expires as an Invalid Date

Suggestion:Maybe can use DatePicker instead of Input in Expires field

changelog(Fixes): Fixed an issue where editing a cookie wrongfully displayed expiry dates as invalid

Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

LGTM!

Before - date would show up as invalid, and upon closing cookie modal would dissappear:
Screenshot 2022-06-24 at 12 26 32

Now - it at least doesn't show up as invalid, and isn't deleted:
Screenshot 2022-06-24 at 12 29 15

It doesn't solve all our problems with cookies, but at least gets rid of the invalid date/disappear behaviour, so from my side I think we can merge. Thank you @tangweikun for contributing this 👍

@tangweikun
Copy link
Contributor Author

LGTM!

Before - date would show up as invalid, and upon closing cookie modal would dissappear: Screenshot 2022-06-24 at 12 26 32

Now - it at least doesn't show up as invalid, and isn't deleted: Screenshot 2022-06-24 at 12 29 15

It doesn't solve all our problems with cookies, but at least gets rid of the invalid date/disappear behaviour, so from my side I think we can merge. Thank you @tangweikun for contributing this 👍

What are the other related issues about cookies? You can show me the issues, I can take a deeper look another day. @filfreire

@filfreire
Copy link
Member

@tangweikun if you wish to have a look, these are some that are open:

@filfreire filfreire requested a review from a team June 29, 2022 10:51
Comment on lines 110 to 111
const isInValidExpires = (val: Cookie['expires']) => val === null || val === '' || new Date(Number(val)).toString() === 'Invalid Date';
if (isInValidExpires(cookie.expires)) {
Copy link
Contributor

@jackkav jackkav Jun 29, 2022

Choose a reason for hiding this comment

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

Its better to use positive naming here isValidDate
also consider using date-fns for validation as we have it installed https://date-fns.org/v2.28.0/docs/isValid

@@ -227,7 +227,7 @@ export class UnconnectedCookieModifyModal extends PureComponent<Props, State> {
</div>
{this._renderInputField(
'expires',
isNaN(new Date(cookie.expires || 0).getTime()) ? 'Invalid Date' : null,
cookie.expires !== null && new Date(Number(cookie.expires)).toString() === 'Invalid Date' ? 'Invalid Date' : null,
Copy link
Contributor

Choose a reason for hiding this comment

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

since this isn't react i'd consider moving it into a condition inside of _renderInputField
if(type==='expires'){
}

@jackkav
Copy link
Contributor

jackkav commented Jun 30, 2022

After testing this PR. It is an improvement on the original but could afford to be more clear about what values are acceptable inputs to the expires field.
It seems currently only a datetime expressed as timeSinceEpoch is accepted. eg. 1672531200000
Which is inconvenient as a user experience.

I'm considering if we should either add support for typing in a GMT/UTCString eg. Thu, 21 Oct 2021 07:28:00 GMT
or perhaps just force the input to use a datetime input.
According to https://developer.mozilla.org/en-US/docs/Web/API/Document/cookie#write_a_new_cookie the tough-cookie library will always create a GMTstring but it can take string | number | Date | null

;max-age=max-age-in-seconds (e.g., 60*60*24*365 or 31536000 for a year)
;expires=date-in-GMTString-format If neither expires nor max-age specified it will expire at the end of session.

For now I think we can ignore the never state, but I've pushed a commit that addresses the above. What do we think? @tangweikun @filfreire

image

@tangweikun
Copy link
Contributor Author

After testing this PR. It is an improvement on the original but could afford to be more clear about what values are acceptable inputs to the expires field. It seems currently only a datetime expressed as timeSinceEpoch is accepted. eg. 1672531200000 Which is inconvenient as a user experience.

I'm considering if we should either add support for typing in a GMT/UTCString eg. Thu, 21 Oct 2021 07:28:00 GMT or perhaps just force the input to use a datetime input. According to https://developer.mozilla.org/en-US/docs/Web/API/Document/cookie#write_a_new_cookie the tough-cookie library will always create a GMTstring but it can take string | number | Date | null

;max-age=max-age-in-seconds (e.g., 60*60*24*365 or 31536000 for a year)
;expires=date-in-GMTString-format If neither expires nor max-age specified it will expire at the end of session.

For now I think we can ignore the never state, but I've pushed a commit that addresses the above. What do we think? @tangweikun @filfreire

image

LGTM!
The interactive experience is better than before.At the same time I think expires row should be made shorter, now the user has to click on the far right to pop up the datepicker, like this:

coo

@jackkav
Copy link
Contributor

jackkav commented Jul 1, 2022

@tangweikun I agree, but I don't think we have a half width convention anywhere else yet.

Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

LGTM!

⚠️ Fyi @jackkav @tangweikun found a few issues:

  • Pressing the Clear button doesn't visible delete the date from the input field (though in the background it does seem to delete it. Pressing Clear button a second time seems clear up the date.

1st clear attempt:
Screenshot 2022-07-04 at 16 15 51

2nd clear attempt:
Screenshot 2022-07-04 at 16 15 57

Finally cleared:
Screenshot 2022-07-04 at 16 16 03

  • When I try to type a date in one of the input parts, like only the year part, I can't seem to write all 4 digits without that part being reset (e.g. try to write 2022).
    image

@jackkav jackkav enabled auto-merge (squash) July 4, 2022 16:17
@jackkav jackkav merged commit 0f5d858 into Kong:develop Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants