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...

mayorova · 2024-12-04T08:27:36Z

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

+    if (fieldDate) {
+      value = fieldDate.toISOString().replace(msExp, 'Z')
+    } else if (selectedItem.id === 5 ) {
+      value = pickedDate.toISOString().replace(msExp, 'Z')
+    }
+
+    return value


Removed the suggestion, because noticed that it's not just "if-else", but "if - else if", and value can be empty at the return.

Maybe we can accept both formats on the backend by using DateTime.parse? It seems to work both with milliseconds and no milliseconds.

I'll explore this possibility, it might work

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

mayorova · 2024-12-04T10:03:14Z

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

+        <span className="pf-c-form-control-expiration-hint">{fieldHint}</span>
+      </FormGroup>
+      <input id={id} name={id} type="hidden" value={dateValue} />
+      {selectedItem.id === 5 && (


I don't love these synthetic ids on the options list. It's a bit hard to understand why this is checked for equality with 5.

Some alternative ideas I had:

use a string instead of the numeric ID for an easier interpretation, e.g.

{ id: '7', name: '7 days', period: 7 }, { id: '30', name: '30 days', period: 30 }, { id: 'custom', name: 'Custom...', period: 0 }, { id: 'no-exp', name: 'No expiration', period: 0 }

or something like that.

Or even remove the id, and reuse period, and have for example period: 0 for "no expiration", and period: -1 for custom. This way there are less values that we need to care about.

I also suggest to use label rather than name, as it is used for the field label in FormSelectOption, so we'll have something like

<FormSelectOption key={item.period} label={item.label} value={item.period} />

use a string instead of the numeric ID for an easier interpretation, e.g.

Done fda4ff2

Or even remove the id, and reuse period, and have for example period: 0 for "no expiration", and period: -1 for custom. This way there are less values that we need to care about.

I didn't do this because I find this as confusing as the previous code.

I also suggest to use label rather than name, as it is used for the field label in FormSelectOption, so we'll have something like

Done 14b083d

mayorova · 2024-12-04T11:16:46Z

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

+      )}
+      {dateValue === '' && (
+        <>
+          <br />


I don't like this break here, but I couldn't find an easy way to add some top margin, so that's OK...

I was looking at https://pf4.patternfly.org/utilities/spacing/, and was hoping that adding className="pf-u-mt-md" would help, but it didn't 😬 I think that probably the CSS containing this class is not being loaded... not sure, but I didn't investigate further.

Also, now I think the parenthesis () are not needed, but well, not important.

For simplicity

Also we don't need to extend from `IRecord`

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.

Add a warning when the time zone differs from the provider's one

Jest complains about wrong syntax ¯\_(ツ)_/¯

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.

Co-authored-by: Daria Mayorova <[email protected]>

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.

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 added 3 commits November 26, 2024 12:14

New React component: ExpirationDatePicker

dc05a73

Add the new component to Access Token forms

4f4690d

Access token UI: Update and add tests

995530a

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

Move access token strings to root level

4036f22

mayorova reviewed Dec 4, 2024

View reviewed changes

jlledom added 9 commits December 5, 2024 10:26

Accept timestamps with milliseconds

d2f7411

Send timestamps with milliseconds

6d88683

For simplicity

Rename ExpirationItem.name to label

14b083d

Also we don't need to extend from `IRecord`

Use more meaningful ids

fda4ff2

Better spacing

667e146

Fix tests after changes

0921951

Show the hint as a helper text

588b314

Hint visible when picking a date from calendar

587a618

Update tests after changes

39b8c24

jlledom added 2 commits December 5, 2024 14:36

Add expiration time to index table

88badaf

Update cukes after changes

bfd0398

jlledom added 4 commits December 12, 2024 12:53

Better hint date format

62d048d

Add a time zone warning

7f45144

Add a warning when the time zone differs from the provider's one

Use OutlinedQuestionCircleIcon instead of HelpIcon

05bc601

Jest complains about wrong syntax ¯\_(ツ)_/¯

Add tests for time zone mismatch warning

6dbcdfe

Fix linter error

63f4496

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 and others added 2 commits December 19, 2024 18:00

Use PF class to add top margin

5a7255e

Co-authored-by: Daria Mayorova <[email protected]>

Apply suggestions from #3954

85abc92

jlledom added 2 commits December 20, 2024 13:28

Calendar: Accept only future dates

6bec27c

Fix tests

25ce970

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-10245: access tokens expiration UI #3943

THREESCALE-10245: access tokens expiration UI #3943

jlledom commented Nov 13, 2024 •

edited

Loading

mayorova commented Dec 3, 2024

mayorova Dec 4, 2024 •

edited

Loading

jlledom Dec 4, 2024

jlledom Dec 5, 2024

jlledom commented Dec 4, 2024 •

edited

Loading

mayorova Dec 4, 2024

jlledom Dec 5, 2024

mayorova Dec 4, 2024

jlledom Dec 5, 2024

mayorova Dec 5, 2024

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

THREESCALE-10245: access tokens expiration UI #3943

Are you sure you want to change the base?

THREESCALE-10245: access tokens expiration UI #3943

Conversation

jlledom commented Nov 13, 2024 • edited Loading

mayorova commented Dec 3, 2024

mayorova Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

jlledom Dec 4, 2024

Choose a reason for hiding this comment

jlledom Dec 5, 2024

Choose a reason for hiding this comment

jlledom commented Dec 4, 2024 • edited Loading

mayorova Dec 4, 2024

Choose a reason for hiding this comment

jlledom Dec 5, 2024

Choose a reason for hiding this comment

mayorova Dec 4, 2024

Choose a reason for hiding this comment

jlledom Dec 5, 2024

Choose a reason for hiding this comment

mayorova Dec 5, 2024

Choose a reason for hiding this comment

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

jlledom commented Nov 13, 2024 •

edited

Loading

mayorova Dec 4, 2024 •

edited

Loading

jlledom commented Dec 4, 2024 •

edited

Loading

jlledom commented Dec 12, 2024 •

edited

Loading

jlledom commented Dec 20, 2024 •

edited

Loading