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

THREESCALE-10245: access tokens expiration UI #3943

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions app/javascript/packs/expiration_date_picker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { ExpirationDatePickerWrapper } from 'AccessTokens/components/ExpirationDatePicker'

import type { Props } from 'AccessTokens/components/ExpirationDatePicker'

import { safeFromJsonString } from '../src/utilities/json-utils'

document.addEventListener('DOMContentLoaded', () => {
const containerId = 'expiration-date-picker-container'
const container = document.getElementById(containerId)

if (!container) {
throw new Error(`Missing container with id "${containerId}"`)
}

const props = safeFromJsonString<Props>(container.dataset.props)

if (!props) {
throw new Error('Missing props')
}

ExpirationDatePickerWrapper(props, containerId)
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.pf-c-calendar-month, .pf-c-form-control-expiration {
width: 50%;
}

.pf-c-form-control-expiration {
margin-right: 1em;
}
126 changes: 126 additions & 0 deletions app/javascript/src/AccessTokens/components/ExpirationDatePicker.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import { useState, useMemo } from 'react'
import { Alert, CalendarMonth, FormGroup, FormSelect, FormSelectOption } from '@patternfly/react-core'

import { createReactWrapper } from 'utilities/createReactWrapper'
import type { IRecord } from 'utilities/patternfly-utils'

import type { FunctionComponent, FormEvent } from 'react'

import './ExpirationDatePicker.scss'

interface ExpirationItem extends IRecord {
id: number;
name: string;
period: number; // In seconds
}

const collection: ExpirationItem[] = [
{ id: 1, name: '7 days', period: 7 },
{ id: 2, name: '30 days', period: 30 },
{ id: 3, name: '60 days', period: 60 },
{ id: 4, name: '90 days', period: 90 },
{ id: 5, name: 'Custom...', period: 0 },
{ id: 6, name: 'No expiration', period: 0 }
]

const dayMs = 60 * 60 * 24 * 1000
const msExp = /\.\d{3}Z$/

interface Props {
id: string;
label: string | null;
}

const ExpirationDatePicker: FunctionComponent<Props> = ({ id, label }) => {
const [selectedItem, setSelectedItem] = useState(collection[0])
const [pickedDate, setPickedDate] = useState(new Date())
const fieldName = `human_${id}`
const fieldLabel = label ?? 'Expires in'

const fieldDate = useMemo(() => {
if (selectedItem.period === 0) return null

return new Date(new Date().getTime() + selectedItem.period * dayMs)
}, [selectedItem])

const fieldHint = useMemo(() => {
if (!fieldDate) return

const date = new Date(fieldDate)
date.setHours(0, 0, 0, 0)

return `The token will expire on ${date.toLocaleDateString()}`
}, [fieldDate])

const dateValue = useMemo(() => {
let value = ''

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

return value
Copy link
Contributor

@mayorova mayorova Dec 4, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll explore this possibility, it might work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}, [fieldDate, selectedItem, pickedDate])

const handleOnChange = (_value: string, event: FormEvent<HTMLSelectElement>) => {
const value = (event.target as HTMLSelectElement).value
const selected = collection.find(i => i.id.toString() === value) ?? null

if (selected === null) return

setSelectedItem(selected)
setPickedDate(new Date())
}

return (
<>
<FormGroup
isRequired
fieldId={fieldName}
label={fieldLabel}
>
<FormSelect
className="pf-c-form-control-expiration"
id={fieldName}
value={selectedItem.id}
onChange={handleOnChange}
>
{collection.map((item: ExpirationItem) => {
return (
<FormSelectOption
key={item.id}
label={item.name}
value={item.id}
/>
)
})}
</FormSelect>
<span className="pf-c-form-control-expiration-hint">{fieldHint}</span>
</FormGroup>
<input id={id} name={id} type="hidden" value={dateValue} />
{selectedItem.id === 5 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

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}
  />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<>
<br />
<CalendarMonth date={pickedDate} onChange={setPickedDate} />
<br />
</>
)}
{dateValue === '' && (
<>
<br />
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

<Alert title="Expiration is recommended" variant="warning">
It is strongly recommended that you set an expiration date for your token to help keep your information
secure
</Alert>
</>
)}
</>
)
}

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

export type { ExpirationItem, Props }
export { ExpirationDatePicker, ExpirationDatePickerWrapper }
11 changes: 11 additions & 0 deletions app/views/provider/admin/user/access_tokens/_form.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,14 @@
= form.input :permission, as: :patternfly_select,
collection: @access_token.available_permissions,
include_blank: false

- if @access_token.persisted?
.pf-c-form__group
.pf-c-form__group-label
label.pf-c-form__label
span.pf-c-form__label-text
= t('access_token_options.expires_at')
.pf-c-form__group-control
= @access_token.expires_at.present? ? l(@access_token.expires_at) : t('access_token_options.no_expiration')
- else
div id='expiration-date-picker-container' data-props={ id: 'access_token[expires_at]', label: t('access_token_options.expires_in') }.to_json
7 changes: 7 additions & 0 deletions app/views/provider/admin/user/access_tokens/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@
dd class="pf-c-description-list__description"
div class="pf-c-description-list__text"
= token.human_permission
div class="pf-c-description-list__group"
dt class="pf-c-description-list__term"
span class="pf-c-description-list__text"
| Expires at
dd class="pf-c-description-list__description"
div class="pf-c-description-list__text"
= token.expires_at.present? ? l(token.expires_at) : t('access_token_options.no_expiration')
div class="pf-c-description-list__group"
dt class="pf-c-description-list__term"
span class="pf-c-description-list__text"
Expand Down
1 change: 1 addition & 0 deletions app/views/provider/admin/user/access_tokens/new.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

- content_for :javascripts do
= javascript_packs_with_chunks_tag 'pf_form'
= javascript_packs_with_chunks_tag 'expiration_date_picker'

div class="pf-c-card"
div class="pf-c-card__body"
Expand Down
3 changes: 3 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ en:
cms: 'Developer Portal API'
ro: 'Read Only'
rw: 'Read & Write'
no_expiration: 'Never expires'
expires_at: 'Expires at'
expires_in: 'Expires in'
notification_category_titles:
account: 'Accounts'
application: 'Applications'
Expand Down
20 changes: 12 additions & 8 deletions features/provider/admin/user/access_tokens.feature
Original file line number Diff line number Diff line change
Expand Up @@ -42,33 +42,37 @@ Feature: Provider Admin Access tokens
Then there is a required field "Name"
And there is a required field "Scopes"
And there is a required field "Permission"
And there is a required field "Expires in"
And the submit button is enabled

Scenario: Create access tokens without required fields
When they press "Create Access Token"
Then field "Name" has inline error "can't be blank"
And field "Scopes" has inline error "select at least one scope"
And field "Permission" has no inline error
And field "Expires in" has no inline error

Scenario: Create access token
When they press "Create Access Token"
And the form is submitted with:
| Name | LeToken |
| Analytics API | Yes |
| Permission | Read & Write |
| Name | LeToken |
| Analytics API | Yes |
| Permission | Read & Write |
| Expires in | No expiration |
Then the current page is the personal tokens page
And they should see the flash message "Access token was successfully created"
And should see the following details:
| Name | LeToken |
| Scopes | Analytics API |
| Permission | Read & Write |
| Name | LeToken |
| Scopes | Analytics API |
| Permission | Read & Write |
| Expires at | Never expires |
And there should be a link to "I have copied the token"

Rule: Edit page
Background:
Given the provider has the following access tokens:
| Name | Scopes | Permission |
| LeToken | Billing API, Analytics API | Read Only |
| Name | Scopes | Permission | Expires at |
| LeToken | Billing API, Analytics API | Read Only | 2030-01-01T00:00:00Z |
And they go to the access token's edit page

Scenario: Navigation to edit page
Expand Down
127 changes: 127 additions & 0 deletions spec/javascripts/AccessTokens/components/ExpirationDatePicker.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import { mount } from 'enzyme'

import { ExpirationDatePicker } from 'AccessTokens/components/ExpirationDatePicker'

import type { ExpirationItem, Props } from 'AccessTokens/components/ExpirationDatePicker'
import type { ReactWrapper } from 'enzyme'

const defaultProps: Props = {
id: 'expires_at',
label: 'Expires in'
}

const mountWrapper = (props: Partial<Props> = {}) => mount(<ExpirationDatePicker {...{ ...defaultProps, ...props }} />)

const selectItem = (wrapper: ReactWrapper<any, Readonly<object>>, item: ExpirationItem) => {
wrapper.find('select.pf-c-form-control-expiration').simulate('change', { target: { value: item.id.toString() } })
}

const pickDate = (wrapper: ReactWrapper<any, Readonly<object>>) => {
/*
* Pick tomorrow, to do so, we get the date selected by default which is today and click the next one.
* It could happen that today is the last day in the calendar, in that case we pick the previous day, yesterday.
* In any case, we return the picked date to the caller.
*/
const targetDate = new Date()
targetDate.setHours(0)
targetDate.setMinutes(0)
targetDate.setSeconds(0)
targetDate.setMilliseconds(0)

const tomorrowButton = wrapper.find('.pf-m-selected + td > button')

if (tomorrowButton.length === 0) {
// No tomorrow, pick yesterday
const dayButtons = wrapper.find('button.pf-c-calendar-month__date')
const yesterdayButton = dayButtons.at(dayButtons.length - 2)

yesterdayButton.simulate('click')
targetDate.setDate(targetDate.getDate() - 1)
return targetDate
}

tomorrowButton.simulate('click')
targetDate.setDate(targetDate.getDate() + 1)
return targetDate
}

it('should render itself', () => {
const wrapper = mountWrapper()
expect(wrapper.exists()).toEqual(true)
})

describe('select a period', () => {
const targetItem: ExpirationItem = { id: 4, name: '90 days', period: 90 }

it('should update hint to the correct date', () => {
const wrapper = mountWrapper()
const targetDate = new Date(new Date().getTime() + 1000 * 60 * 60 * 24 * targetItem.period)
const expectedHint = `The token will expire on ${targetDate.toLocaleDateString()}`

selectItem(wrapper, targetItem)
const hint = wrapper.find('.pf-c-form-control-expiration-hint').text()

expect(hint).toBe(expectedHint)
})

it('should update hidden input value to the correct timestamp', () => {
const wrapper = mountWrapper()
const targetDate = new Date(new Date().getTime() + 1000 * 60 * 60 * 24 * targetItem.period)
const expectedValue = targetDate.toISOString().replace(/\.\d{3}Z$/, 'Z')

selectItem(wrapper, targetItem)
const value = wrapper.find(`input#${defaultProps.id}`).prop('value')

expect(value).toBe(expectedValue)
})
})

describe('select "Custom"', () => {
const targetItem: ExpirationItem = { id: 5, name: 'Custom...', period: 0 }

it('should show a calendar', () => {
const wrapper = mountWrapper()

selectItem(wrapper, targetItem)
const calendar = wrapper.find('.pf-c-calendar-month')

expect(calendar.exists()).toBe(true)
})

describe('pick a date from the calendar', () => {
it('should update hidden input value to the correct timestamp', () => {
const wrapper = mountWrapper()

selectItem(wrapper, targetItem)
const targetDate = pickDate(wrapper)
const expectedValue = targetDate.toISOString().replace(/\.\d{3}Z$/, 'Z')
const value = wrapper.find(`input#${defaultProps.id}`).prop('value')

expect(value).toBe(expectedValue)
})
})
})

describe('select "No expiration"', () => {
const targetItem: ExpirationItem = { id: 6, name: 'No expiration', period: 0 }

it('should show a warning', () => {
const wrapper = mountWrapper()

selectItem(wrapper, targetItem)
const warning = wrapper.find('.pf-c-alert.pf-m-warning')

expect(warning.exists()).toBe(true)
})

it('should update hidden input value to empty', () => {
const wrapper = mountWrapper()
const expectedValue = ''

selectItem(wrapper, targetItem)
const value = wrapper.find(`input#${defaultProps.id}`).prop('value')

expect(value).toBe(expectedValue)
})
})