THREESCALE-10245: access tokens expiration UI #3943

jlledom · 2024-11-13T15:32:55Z

What this PR does / why we need it:

After we merged #3939 now we want to update the Access Tokens screens to add a new field for the expiration date. For this, I created a new React component based on what Github does:

A select input with the next options:
- Expires in 7 days
- In 30 days
- In 60 days
- In 90 days
- Custom date
  - A calendar appears to select the desired date
- No expiration
  - An warn is shown to advise the user to set an expiration date for security reasons

Which issue(s) this PR fixes

THREESCALE-10245

Verification steps

Just set the an expiration date when creating a token and verify the data is correctly stored in DB and is properly shown in #index and #edit screens

Screenshots

app/views/provider/admin/user/access_tokens/_form.html.slim

mayorova · 2024-12-03T17:22:50Z

Great job @jlledom ! 🏆
I'm still reviewing the implementation, but a couple of comments so far:

The selected date in the hint on the form is a bit confusing...

Depending if it's US or not, the date can be interpreted differently.

I think the following presentation as it shows on the different screen is better.

Also, I think it would be nice to show the expiration date on the table listing all tokens...

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

jlledom · 2024-12-04T08:44:41Z

Ideally, this provider setting should be the one source of truth for time zone and the react component should use that time zone instead of the browser's one. I was trying to fix it but I gave up, I don't remember why, but I could give it a try again because it's generating some confusion when there's difference between provider and browser timezones.

I couldn't do it. Basically I though on two approaches:

Passing the provider time zone to the Patternfly calendar component
Translating whatever datetime the client sends to provider timezone

Both failed. 1 can't be done because the Patternfly react component doesn't accept ant time zone, it will always use the OS timezone.
2 can't be done because at the moment of the value assignment (calling expires_at=) the owner attribute is nil, so we can't access the provider time zone.

Also, I think it would be nice to show the expiration date on the table listing all tokens...

Done 88badaf

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

jlledom · 2024-12-05T13:51:45Z

From #3951:

Using helper text (under the select) instead of the custom span element to the right (which dos not look very Patternfly-y). The drawback is that when the select is expanded, the value is not seen, but I think it doesn't matter.

588b314

Changing the formatting of the "current value". Not sure it's a nice one (probably too long), but I think it's clear, and it also includes timestamp (but not sure if needed)

I didn't do this one because I think the current format is clear enough and it's formatted in client's time zone. Not sure if using UTC instead would make things clearer, as UTC time can belong to a different day to the client time zone.

When the date is selected via picker, the selected value is also shown now.

587a618

The default value when using "Custom..." is set to tomorrow
The past dates are not pickable in the date picker.

I agree on these two, but I didn't applied them because they make tests much more difficult. Check pickDate on the test suite.

mayorova · 2024-12-09T16:14:06Z

I agree on these two, but I didn't applied them because they make tests much more difficult. Check pickDate on the test suite.

Ooh, well, that's sad, because I think they are very useful, especially not being able to pick later dates - because it doesn't make sense.

mayorova · 2024-12-09T16:23:16Z

I didn't do this one because I think the current format is clear enough and it's formatted in client's time zone. Not sure if using UTC instead would make things clearer, as UTC time can belong to a different day to the client time zone.

Hm, well, I think it's confusing... I don't know where the locale setting for date.toLocaleDateString() is coming from... I've tried to change the language in my Chrome to non-US English, and I keep seeing 12/9/2024 for today. To me it's very confusing.

I'd prefer at least 9 Dec, 2024 or December 9, 2024 like on some other screens. And I still think that adding time could also be useful, because the actual expiry date does include time also, so for the customer it could be useful to know whether the token will expire at 00:01 or 23:59 of the same date.

jlledom · 2024-12-12T08:53:52Z

Hm, well, I think it's confusing... I don't know where the locale setting for date.toLocaleDateString() is coming from...

I'd say it comes from the OS.

I'd prefer at least 9 Dec, 2024 or December 9, 2024 like on some other screens.

Which screens? maybe those screens take the locale from the provider settings.

jlledom · 2024-12-12T09:11:34Z

I'd prefer at least 9 Dec, 2024 or December 9, 2024 like on some other screens.

Which screens? maybe those screens take the locale from the provider settings.

I correct myself, only the timezone can be configured for the provider, locale is always the same in porta for everybody. However, we don't use a single date format in all screens.

I think I'm gonna implement one of your suggestions.

jlledom · 2024-12-12T13:35:39Z

@mayorova I updated the date format in the field hint.

Also, since I don't think we can, or should, fix the time zone mismatch problem, I added a warning to clarify which time will the token expire at.

The tooltip only appears when there's a timezone mismatch between the browser and the provider configuration.

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

mayorova · 2024-12-17T13:48:41Z

Thank you for applying the suggestions!

I have some more 😬 #3954

Basically, when first reviewing (and actually at each consequent review) I had a hard time understanding, what each variable is for...
I also didn't like double-transforming the variable.

So, I am suggesting some refactoring.

Also, I think some suggestions from #3951 are still relevant...

Specifically,

The default value when using "Custom..." is set to tomorrow
The past dates are not pickable in the date picker.

I remember that complicated the tests... but maybe we can somehow still incorporate this?

I think it's not a great UX, if you can set past dates, thus making the token invalid automatically.

jlledom · 2024-12-19T17:37:02Z

Thank you for applying the suggestions!

I have some more 😬 #3954

Basically, when first reviewing (and actually at each consequent review) I had a hard time understanding, what each variable is for... I also didn't like double-transforming the variable.

So, I am suggesting some refactoring.

I applied your suggestions with great joy and rejoicing.

Also, I think some suggestions from #3951 are still relevant...

Specifically,

The default value when using "Custom..." is set to tomorrow
The past dates are not pickable in the date picker.

I remember that complicated the tests... but maybe we can somehow still incorporate this?

I'll try it again then...

jlledom · 2024-12-20T13:17:50Z

@mayorova I applied the suggestions from #3951. I think tests are less relevant now, because in the calendar we pick the same day already picked, but it's the only reasonable way to go IMO.

josemigallas

I am sorry... 😅 reviewers will review!

app/javascript/packs/expiration_date_picker.ts

app/javascript/src/AccessTokens/components/ExpirationDatePicker.scss

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

app/views/provider/admin/user/access_tokens/new.html.slim

josemigallas · 2025-01-08T09:19:04Z

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

+  const dropdownDate = useMemo(() => {
+    if (dropdownSelectedItem.period === 0) return null
+
+    return new Date(today.getTime() + dropdownSelectedItem.period * dayMs)
+  }, [dropdownSelectedItem])
+
+  const selectedDate = useMemo(() => {
+    let value = null
+
+    if (dropdownDate) {
+      value = dropdownDate
+    } else if (dropdownSelectedItem.id === 'custom' ) {
+      value = calendarPickedDate
+    }
+
+    return value
+  }, [dropdownDate, dropdownSelectedItem, calendarPickedDate])
+
+  const formattedDateValue = useMemo(() => {
+    if (!selectedDate) return
+
+    const formatter = Intl.DateTimeFormat('en-US', {
+      month: 'long', day: 'numeric', year: 'numeric', hour: 'numeric', minute: 'numeric', hour12: false
+    })
+
+    return formatter.format(selectedDate)
+  }, [selectedDate])
+
+  const fieldHint = useMemo(() => {
+    if (!formattedDateValue) return
+
+    return `The token will expire on ${formattedDateValue}`
+  }, [formattedDateValue])
+
+  const inputDateValue = useMemo(() => {
+    if (!selectedDate) return ''
+
+    return selectedDate.toISOString()
+  }, [selectedDate])
+
+  const tzMismatch = useMemo(() => {
+    if (tzOffset === undefined) return
+
+    // Timezone offset in the same format as ActiveSupport
+    const jsTzOffset = new Date().getTimezoneOffset() * -60
+
+    return jsTzOffset !== tzOffset
+  }, [tzOffset])
+
+  const labelIcon = useMemo(() => {
+    if (!tzMismatch) return
+
+    return (
+      <Popover
+        bodyContent={(
+          <p>
+            Your local time zone differs from the provider default.
+            The token will expire at the time you selected in your local time zone.
+          </p>
+        )}
+        headerContent={(
+          <span>Time zone mismatch</span>
+        )}
+      >
+        <button
+          aria-describedby="form-group-label-info"
+          aria-label="Time zone mismatch warning"
+          className="pf-c-form__group-label-help"
+          type="button"
+        >
+          <OutlinedQuestionCircleIcon noVerticalAlign />
+        </button>
+      </Popover>
+    )
+  }, [tzMismatch])


Not sure what's the rationale here, but using useMemo is generally a bad idea. Memoization in React is nothing like Rails' (or other proper languages) and in the end it makes it less performant.

I use it to declare a variable as reactive, e.g.:

const selectedDate = useMemo(() => { ... }, [dropdownDate, dropdownSelectedItem, calendarPickedDate])

selectedDate must be re-evaluated whenever dropdownDate, dropdownSelectedItem or calendarPickedDate change.

Is this not the right way to do it?

useMemo is for memoization of very heavy computations. What you want you usually do in the onChange methods as a callback or simply declaring the variables in the body of the component. Every time there is a change, the component will re-render and every variable re-evaluated.

For instance inputDateValue depends directly on selectedDate, so you simply do:

const inputDateValue = selectedDate?.toISOString() // -> string | undefined

I refactored here: 4d8a47c

josemigallas · 2025-01-08T09:29:15Z

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

+  const today: Date = new Date()
+  const tomorrow: Date = new Date(today)
+  tomorrow.setDate(today.getDate() + 1)


This is be redefined every time the component re-renders, do we want this? Otherwise I would suggest creating a state.

const [today, _] = useState(new Date())

In which event does the component re-render? only when the screen is loaded? or once after every state change?

In which event does the component re-render? only when the screen is loaded? or once after every state change?

With every state change.

Thanks, I didn't know this and that's crucial to understand how the whole thing works.

josemigallas · 2025-01-08T09:31:52Z

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

+  )
+}
+
+const ExpirationDatePickerWrapper = (props: Props, containerId: string): void => { createReactWrapper(<ExpirationDatePicker id={props.id} label={props.label} tzOffset={props.tzOffset} />, containerId) }


You can use prop spread here, it used to be dangerous but Typescript is able to check the type.

Suggested change

const ExpirationDatePickerWrapper = (props: Props, containerId: string): void => { createReactWrapper(<ExpirationDatePicker id={props.id} label={props.label} tzOffset={props.tzOffset} />, containerId) }

const ExpirationDatePickerWrapper = (props: Props, containerId: string): void => { createReactWrapper(<ExpirationDatePicker {...prop} />, containerId) }

Yes, I saw that in other components, but when I try to do it here I get a linter error: react/jsx-props-no-spreading

Yeah, that rule was enforced back when we didn't use Typescript but Flow. Type checking was so bad that doing the spread will not check for missing/incorrect props. But Typescript does do that, so feel free to disable that linter error like it's done in other files.

josemigallas · 2025-01-08T09:34:46Z

app/views/provider/admin/user/access_tokens/_form.html.slim

@@ -7,3 +7,14 @@
 = form.input :permission, as: :patternfly_select,
                          collection: @access_token.available_permissions,
                          include_blank: false
+
+- if @access_token.persisted?


This is the kind of stuff that belongs to the presenter IMHO. Also, it's not recommended to use instance variables in partials, pass it as a local instead.

The form is loaded from #new and #edit actions, and they would probably need a different presenter each. I didn't move this logic to a presenter to avoid creating a new presenter for #edit and having to duplicate a new method access_token_persisted? in both presenters.

I moved the instance var to a local, though: 4acb540

app/views/provider/admin/user/access_tokens/_form.html.slim

josemigallas · 2025-01-22T09:55:29Z

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

+  const fieldLabel = label ?? 'Expires in'
+
+  const handleOnChange = (_value: string, event: FormEvent<HTMLSelectElement>) => {
+    const value = (event.target as HTMLSelectElement).value


Aren't event.target.value and value the same?

Yes: 3047613

josemigallas

Code-wise it looks good to me, but I didn't test the branch locally.

mayorova · 2025-01-22T14:29:25Z

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

+  const formattedDateValue = computeFormattedDateValue(selectedDate)
+  const fieldHint = computeFieldHint(formattedDateValue)
+  const tzMismatch = computeTzMismatch(tzOffset)
+  const labelIcon = computeLabelIcon(tzMismatch)


I would probably suggest a more specific name to clarify what this is for, e.g. tzMilsmatchIcon and computeTzMismatchIcon

mayorova · 2025-01-22T14:38:38Z

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

+  // Timezone offset in the same format as ActiveSupport
+  const jsTzOffset = new Date().getTimezoneOffset() * -60


Wow, that's confusing 😅 Not only they are not in the same granularity (seconds vs minutes), but also different signs (negative vs positive) 🤪

mayorova · 2025-01-22T14:43:43Z

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx

+    <Popover
+      bodyContent={(
+        <p>
+            Your local time zone differs from the provider default.


I don't quite like the reference to "provider" here, I think it's confusing.
Maybe we can say "Your local time zone differs from the account settings." ?

Also, this feature is new since I reviewed 😅 I don't think it's necessary, honestly. The time zone in account details only affects analytics graphs, I believe... (or maybe I'm wrong, and it affects something else). But as it's already implemented, I think it's OK.

The problem is we have three timezones here:

DB records are stored in UTC

Account settings timezone

Browser timezone

e.g. If a provider has timezone set to UTC-3, the browser has timezone UTC+1 and you select February 11, 2031, 00:00, You will see this in the browser:

This after sending the form:

And this in the DB:

Those three times are the same moment but in different TZs. It's confusing for the user. I tried to configure the patternfly calendar to receive the account timezone and work on that TZ but it's not possible AFAIK. So I added the warning to heads up the user that they might see different timestamps than the one they selected, but that's ok and the actual moment when the token will expire is what they selected in the form.

e.g. If a provider has timezone set to UTC-3, the browser has timezone UTC+1 and you select February 11, 2031, 00:00, You will see this in the browser:

Oh, OK... I didn't know that this timestamp in the view/index templates would show in the provider account TZ?... I thought it wouldn't be converted, and would be shown in UTC. Yeah, then it makes sense. I guess that's because of

porta/app/controllers/application_controller.rb

Lines 123 to 131 in 15d75c5

def set_timezone

if current_account && (current_account.provider? || current_account.master?)

Time.zone = current_account.timezone if current_account

elsif current_account

Time.zone = current_account.provider_account.timezone

else

Time.zone = Time.zone_default

end

end

mayorova · 2025-01-22T15:05:46Z

Other than a couple of inline comments - it looks good!
(disclaimer: I didn't review the tests carefully, I trust you 😬 )

jlledom · 2025-01-23T10:02:24Z

@mayorova I implemented your suggestions

mayorova

Great job @jlledom !

jlledom requested a review from lvillen November 13, 2024 15:33

jlledom self-assigned this Nov 13, 2024

jlledom changed the base branch from master to THREESCALE-10245-expire-access-tokens November 13, 2024 15:33

jlledom changed the title ~~Threescale 10245 access tokens UI~~ THREESCALE-10245: access tokens expiration UI Nov 13, 2024

jlledom force-pushed the THREESCALE-10245-expire-access-tokens branch from 058be50 to 54db7d7 Compare November 26, 2024 08:58

Base automatically changed from THREESCALE-10245-expire-access-tokens to master November 26, 2024 09:11

jlledom force-pushed the THREESCALE-10245-access-tokens-UI branch from c9d2ba3 to 995530a Compare November 26, 2024 11:15

jlledom marked this pull request as ready for review November 26, 2024 11:37

jlledom requested review from mayorova and akostadinov December 2, 2024 07:53

mayorova reviewed Dec 3, 2024

View reviewed changes

app/views/provider/admin/user/access_tokens/_form.html.slim Outdated Show resolved Hide resolved

mayorova mentioned this pull request Dec 3, 2024

Some suggestions for access token expiration selector #3951

Closed

mayorova reviewed Dec 4, 2024

View reviewed changes

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx Outdated Show resolved Hide resolved

mayorova reviewed Dec 4, 2024

View reviewed changes

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx Outdated Show resolved Hide resolved

mayorova reviewed Dec 4, 2024

View reviewed changes

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx Outdated Show resolved Hide resolved

jlledom requested a review from mayorova December 12, 2024 15:54

mayorova reviewed Dec 17, 2024

View reviewed changes

app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx Outdated Show resolved Hide resolved

jlledom requested a review from mayorova January 7, 2025 11:00

josemigallas requested changes Jan 8, 2025

View reviewed changes

jlledom requested a review from josemigallas January 13, 2025 10:23

josemigallas reviewed Jan 22, 2025

View reviewed changes

josemigallas previously approved these changes Jan 22, 2025

View reviewed changes

jlledom dismissed josemigallas’s stale review via 7fd6dae January 22, 2025 12:28

jlledom requested a review from josemigallas January 22, 2025 12:28

mayorova reviewed Jan 22, 2025

View reviewed changes

jlledom requested a review from mayorova January 23, 2025 09:46

mayorova approved these changes Jan 23, 2025

View reviewed changes

jlledom added 4 commits January 23, 2025 12:46

Access Tokens: Accept timestamps with milliseconds

2f5a027

New React component: ExpirationDatePicker

b3b546c

Add the new component to Access Token forms

6036735

Access token UI: Update and add tests

70bf3ec

jlledom force-pushed the THREESCALE-10245-access-tokens-UI branch from 665c6d7 to 70bf3ec Compare January 23, 2025 11:54

jlledom merged commit 86ec9f9 into master Jan 23, 2025
17 of 21 checks passed

jlledom deleted the THREESCALE-10245-access-tokens-UI branch January 23, 2025 12:24

	const ExpirationDatePickerWrapper = (props: Props, containerId: string): void => { createReactWrapper(<ExpirationDatePicker id={props.id} label={props.label} tzOffset={props.tzOffset} />, containerId) }
	const ExpirationDatePickerWrapper = (props: Props, containerId: string): void => { createReactWrapper(<ExpirationDatePicker {...prop} />, containerId) }

		// Timezone offset in the same format as ActiveSupport
		const jsTzOffset = new Date().getTimezoneOffset() * -60

	def set_timezone
	if current_account && (current_account.provider? \|\| current_account.master?)
	Time.zone = current_account.timezone if current_account
	elsif current_account
	Time.zone = current_account.provider_account.timezone
	else
	Time.zone = Time.zone_default
	end
	end

THREESCALE-10245: access tokens expiration UI #3943

THREESCALE-10245: access tokens expiration UI #3943

Conversation

jlledom commented Nov 13, 2024 • edited Loading

mayorova commented Dec 3, 2024

jlledom commented Dec 4, 2024 • edited Loading

jlledom commented Dec 5, 2024

mayorova commented Dec 9, 2024

mayorova commented Dec 9, 2024

jlledom commented Dec 12, 2024 • edited Loading

jlledom commented Dec 12, 2024

jlledom commented Dec 12, 2024

mayorova commented Dec 17, 2024

jlledom commented Dec 19, 2024

jlledom commented Dec 20, 2024 • edited Loading

josemigallas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas Jan 10, 2025 • edited Loading

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

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

josemigallas Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas left a 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

mayorova commented Jan 22, 2025

jlledom commented Jan 23, 2025

mayorova left a comment

Choose a reason for hiding this comment

jlledom commented Nov 13, 2024 •

edited

Loading

jlledom commented Dec 4, 2024 •

edited

Loading

jlledom commented Dec 12, 2024 •

edited

Loading

jlledom commented Dec 20, 2024 •

edited

Loading

josemigallas Jan 10, 2025 •

edited

Loading

josemigallas Jan 22, 2025 •

edited

Loading