Skip to content

GPU-2631 UI: Add UI for Api Token Management - #855

Open
dominikschatz wants to merge 5 commits into
f-gpu-2597from
f-gpu-2631
Open

GPU-2631 UI: Add UI for Api Token Management#855
dominikschatz wants to merge 5 commits into
f-gpu-2597from
f-gpu-2631

Conversation

@dominikschatz

Copy link
Copy Markdown
Contributor

GPU-2631 UI: Add UI for Api Token Management


await expect(form).toBeVisible();

const submitBtn = page.locator(API_CREATE_MODAL).locator(".modal-footer").locator("button.primary");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add action attributes (like data-action="confirm") to the buttons like other modals.
For confirming the modal, please use the clickModalAction helper then.


await submitBtn.click();

await expect(page.locator(".success-message")).toBeVisible();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add an id to the success-notification (NotificationService/I18nNotificationService.show), and then use the findNotification helper to get the notification by the ID.


await expect(page.locator(API_DELETE_MODAL)).toBeVisible();

await page.locator(API_DELETE_MODAL).locator("button.alert").click();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use actions here as well


const row = table.locator(".data-row").first();

const deleteButton = row.locator('[title="editor.tagtype_delete_label"]').first();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use the findTableAction helper instead, and provide the delete action-id instead.


await row.locator('gtx-checkbox').click();

await row.locator('button.alert').click();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use the findTableAction helper


await expect(page.locator(API_DELETE_MODAL)).toBeVisible();

await page.locator(API_DELETE_MODAL).locator("button.alert").click();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

action-attribute

@deckdom deckdom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally please check all the formatting of the ts filles.
VSCode lights up like a christmas tree on every file.
ESLint should be able to fix most if not all formatting issues anyways.

});

async function openApiTokenModal(page: Page) {
await page.locator(".ng-trigger-toggleState").click();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't use angular internal ng prefixed classes for selectors.
Especially not trigger classes, as these are for animations.

Use the openContext function instead (See changes for the user-menu` first):

const menu = page.locator('gtx-user-menu');
const dropdown = await openContext(menu.locator('.user-name gtx-dropdown-list'));
await dropdown.locator('[data-action="manage-api-tokens"]').click();

<div>
<span [innerHTML]="'user.logged_in_as' | gtxI18n:{ name: getUserName() }"></span>
<span [innerHTML]="'user.logged_in_as' | gtxI18n"></span>
<gtx-dropdown-list>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add data-context-id attributes to the gtx-dropdown-list. and gtx-dropdown-content elements (same ID), and the data-context-trigger .
This allows for the openContext helper to properly detect and relate the elements together.

<gtx-dropdown-item overrideSlot="changePasswordOption" (click)="showPasswordModalClicked()">
{{ 'user.change_password_label' | gtxI18n }}
</gtx-dropdown-item>
<gtx-dropdown-item overrideSlot="manageApiTokensOption" (click)="showApiTokensModalClicked()">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do not add a overrideSlot here.
Add a data-action attribute for later use instead.


async function openApiTokenCreateModal(page: Page) {
const modalTable = page.locator(API_MODAL).locator("gtx-manage-api-tokens-table");
const createNewBtn = modalTable.locator(".entity-table-actions-bar button");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add data-action attribute

await createNewBtn.click();
}

async function clearEntries(IMPORTER: EntityImporter) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Create a dedicated test-user in the setup instead - See most of the editor-ui tests.
This removes the need to cleanup these tokens and we can do permission checks.


export interface ApiTokenListResponse extends ListResponse<ApiTokenResponse> { }

export interface ApiTokenResponse extends Response {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are all fields re-defined??
We have the model already defined, reuse them.

data: TokenData
}

interface TokenData {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Call it ApiTokenData

valid: boolean;
}

export interface ApiTokenData {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delete this - This is duplicated again and is the same as the TokenData from the other file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this service - The cms-components doesn't provide yet another abstraction for the rest-client.
Simply use the rest-client in the modal/table directly, as it's only 2 endpoints anyways that we use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delete this - The cms-rest-clients-angular library is deprecated, and we'll not add new functionality to it, as we want to get rid of it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rename to api-tokens-properties.component.ts (and html/scss file)

})
export class ApiTokensCreateFormComponent extends BaseFormPropertiesComponent<EditableApiToken> {
@Input()
control: FormControl<EditableApiToken>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this - This component doesn't need the control, as this control is the form.

The structure should look like this:

  • Modal Component
    -- Has a control property (i.E. this property here)
    -- Template then contains <gtx-api-tokens-properties [formControl]="control" />

The Modal component can now check the validity of the entire form via control.valid and get the value via control.value.

readonly apiTokenNames: Array<string>;

@Output()
validChange = new EventEmitter<boolean>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this as well

super(changeDetector);
}

override ngOnInit(): void {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Entire override can be caned as well, as the two extra subscriptions aren't needed

[animate]="true"
/>
</p>
<gtx-copy-value

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The modal itself should only do one thing - House the properties component and create the new token.
This keeps the logic all grouped together and we can reuse more parts.
When the token was created, simply close the modal and open a dedicated copy-value-modal instead.
Note: The component which opened the api-tokens-create-modal should handle the opening of the copy-value-modal.

}

export type EditableApiTokenPackage = Pick<TokenData, 'name' | 'expires'>; No newline at end of file
export type EditableApiToken = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use type inheritance for this:

export interface EditableApiToken {
  name: string;
  expires?: string;
}

export interface ApiTokenData extends EditableApiToken {
  readonly cdate: number;
  // etc.
}


interface TokenData {
export interface ApiTokenData {
token: string;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The token value is only visible in the ApiTokenCreateResponse, therefore please remove it from here and add it to the Response-Model instead.


getApiTokens: () => ListResponse<ApiTokenResponse>;
addApiTokens: (body: ApiTokenCreateRequest) => ApiTokenResponse;
getApiTokens: (pageSize?: number, page?: number, sort?: string) => ListResponse<ApiTokenResponse>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rename it to listApiTokens instead, to make the name consistent with the naming of the other functions (at least as well as we can).
Create a ApiTokenListOptions Model in the request.ts with these properties instead and provide it as object instead, like all other "list" functions do.

modifyPublishQueue: (body) => this.executeMappedJsonRequest(POST, '/admin/content/publishqueue', body),

getApiTokens: () => this.executeMappedJsonRequest(GET, '/admin/token'),
getApiTokens: (pageSize: number = 10, page: number = 1, sort: string = '-cdate') => this.executeMappedJsonRequest(GET, `/admin/token?pageSize=${pageSize}&page=${page}&sort=${sort}`),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't simply append the options yourself. There's an extra argument you may pass, i.E.
this.executeMappedJsonRequest(GET, '/admin/token', null, params)

return { str, regex };
}

export function futureDateValidator(control: AbstractControl): ValidationErrors | null {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should probably have an extra option if the time should be cleared.
On default we shouldn't clear the time (i.E. line 219), as otherwise we would allow someone to create a token which is already expired (or whereever this validator is also going to be used in the future)

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