ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: ui/app/models/database/role.js (JavaScript)

Code before:
 */

import Model, { attr } from '@ember-data/model';
import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
import { getRoleFields } from 'vault/utils/model-helpers/database-helpers';
import { expandAttributeMeta } from 'vault/utils/field-to-attrs';
import { withModelValidations } from 'vault/decorators/model-validations';

const validations = {
  database: [
    {
      validator(model) {
        const { database } = model;
        if (database) return true;
      },
      message: 'Database is required',
    },
  ],
  type: [
    {
      validator(model) {
        const { type } = model;
        if (type) return true;
      },
      message: 'role type is required',
    },
  ],
  username: [
    {
      validator(model) {
        const { type, username } = model;
        if (!type || type === 'dynamic') return true;
        if (username) return true;
      },
      message: 'username is required',
    },
  ],
};
@withModelValidations(validations)
export default class RoleModel extends Model {
  idPrefix = 'role/';

  @attr('string', {
    readOnly: true,
  })
  backend;

  @attr('string', {
    label: 'Role name',
  })
  name;

Review comment (suggestion):
```suggestion
    { type: 'presence', message: 'Role type is required' },
```

Code after:
 */
import Model, { attr } from '@ember-data/model';
import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
import { getRoleFields } from 'vault/utils/model-helpers/database-helpers';
import { expandAttributeMeta } from 'vault/utils/field-to-attrs';
import { withModelValidations } from 'vault/decorators/model-validations';
const validations = {
  database: [{ type: 'presence', message: 'Database is required.' }],
  type: [{ type: 'presence', message: 'Type is required.' }],
  username: [
    {
      validator(model) {
        const { type, username } = model;
        if (!type || type === 'dynamic') return true;
        if (username) return true;
      },
      message: 'Username is required.',
    },
  ],
};
@withModelValidations(validations)
export default class RoleModel extends Model {
  idPrefix = 'role/';
  @attr('string', { readOnly: true }) backend;
  @attr('string', { label: 'Role name' }) name;
  @attr('array', {
    label: 'Connection name',
    editType: 'searchSelect',
    fallbackComponent: 'string-list',
    models: ['database/connection'],
    selectLimit: 1,
    onlyAllowExisting: true,
    subText: 'The database connection for which credentials will be generated.',
  })
  database;
  @attr('string', {
    label: 'Type of role',
    noDefault: true,
    possibleValues: ['static', 'dynamic'],
  })
  type;
  @attr({
    editType: 'ttl',
    defaultValue: '1h',
    label: 'Generated credentials’s Time-to-Live (TTL)',
    helperTextDisabled: 'Vault will use a TTL of 1 hour.',
    defaultShown: 'Engine default',
  })
  default_ttl;
  @attr({
    editType: 'ttl',
Repository: hashicorp/vault
File: ui/app/components/database-role-setting-form.js (JavaScript)

Code before:
 * @param {string} [mode=create] - mode of the form (eg. create or edit)
 * @param {object} dbParams - holds database config values, (plugin_name [eg 'mongodb-database-plugin'], skip_static_role_rotation_import)
 */

export default class DatabaseRoleSettingForm extends Component {
  get dbConfig() {
    return this.args.dbParams;
  }

  get settingFields() {
    const dbValues = this.args.dbParams;
    if (!this.args.roleType) return null;
    const dbValidFields = getRoleFields(this.args.roleType);
    return this.args.attrs.filter((a) => {
      // Sets default value for skip_import_rotation based on parent db config value
      if (a.name === 'skip_import_rotation' && this.args.mode === 'create') {
        a.options.defaultValue = dbValues?.skip_static_role_rotation_import;
      }
      return dbValidFields.includes(a.name);
    });
  }

  get statementFields() {
    const type = this.args.roleType;
    if (!type) return null;
    const dbValidFields = getStatementFields(type, this.dbConfig ? this.dbConfig.plugin_name : null);
    return this.args.attrs.filter((a) => {
      return dbValidFields.includes(a.name);
    });
  }

  get isOverridden() {
    if (this.args.mode !== 'create' || !this.dbConfig) return null;

    const dbSkip = this.dbConfig.skip_static_role_rotation_import;
    const staticVal = this.args.model.get('skip_import_rotation');
    return this.args.mode === 'create' && dbSkip !== staticVal;
  }
}


Review comment (suggestion):
will `plugin_name` always exist or should we explicitly check that it does here? 

`this.dbConfig?.plugin_name || null`

Code after:
 * @param {string} [mode=create] - mode of the form (eg. create or edit)
 * @param {object} dbParams - holds database config values, { plugin_name: string [eg 'mongodb-database-plugin'], skip_static_role_rotation_import: boolean }
 */

export default class DatabaseRoleSettingForm extends Component {
  get dbConfig() {
    return this.args.dbParams;
  }

  get settingFields() {
    const dbValues = this.args.dbParams;
    if (!this.args.roleType) return null;
    const dbValidFields = getRoleFields(this.args.roleType);
    return this.args.attrs.filter((a) => {
      // Sets default value for skip_import_rotation based on parent db config value
      if (a.name === 'skip_import_rotation' && this.args.mode === 'create') {
        a.options.defaultValue = dbValues?.skip_static_role_rotation_import;
      }
      return dbValidFields.includes(a.name);
    });
  }

  get statementFields() {
    const type = this.args.roleType;
    if (!type) return null;
    const dbValidFields = getStatementFields(type, this.dbConfig ? this.dbConfig.plugin_name : null);
    return this.args.attrs.filter((a) => {
      return dbValidFields.includes(a.name);
    });
  }

  get isOverridden() {
    if (this.args.mode !== 'create' || !this.dbConfig) return null;

    const dbSkip = this.dbConfig.skip_static_role_rotation_import;
    const staticVal = this.args.model.get('skip_import_rotation');
    return this.args.mode === 'create' && dbSkip !== staticVal;
  }
}
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

import Component from '@glimmer/component';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';
import { service } from '@ember/service';

/**
 * @module TotpEdit
 * `TotpEdit` is a component that allows you to create, view or delete a TOTP key or view the QR code of the key.
 *
 * @example
 * ```js
 *   <TotpEdit @model={{this.model}} @mode={{this.mode}} />
 * ```
 * @param {object} model - The totp edit model.
 * @param {string} mode - The mode to render. Either 'create' or 'show'.
 */
const LIST_ROOT_ROUTE = 'vault.cluster.secrets.backend.list-root';
const SHOW_ROUTE = 'vault.cluster.secrets.backend.show';

export default class TotpEdit extends Component {
  @service router;

  @tracked hasGenerated = false;
  @tracked invalidFormAlert = '';
  @tracked modelValidations;

  successCallback;

  get keyFormFields() {
    const shared = ['name', 'generate', 'issuer', 'accountName'];
    const generated = [...shared, 'exported'];
    const nonGenerated = [...shared, 'url', 'key'];
    return this.args.model.generate ? generated : nonGenerated;
  }

  get mode() {
    return this.args.mode || 'show';
  }

Review comment (suggestion):
We have a lot of bad jsdoc syntax floating around, in order to get components to properly render in docfy these backticks should be removed (this component isn't useful to render there, so this is mostly an FYI
```suggestion
 *   <TotpEdit @model={{this.model}} @mode={{this.mode}} />
```

Code after:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

import Component from '@glimmer/component';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';
import { service } from '@ember/service';
import { task } from 'ember-concurrency';
import { waitFor } from '@ember/test-waiters';

/**
 * @module TotpEdit
 * `TotpEdit` is a component that allows you to create, view or delete a TOTP key.
 * When creating a key if `generate` and `exported` are true then after a successful save the UI renders a QR code for the generated key.
 * @example
 *   <TotpEdit @model={{this.model}} @mode={{this.mode}} />
 *
 * @param {object} model - The totp key ember data model.
 * @param {string} mode - The mode to render. Either 'create' or 'show'.
 */
const LIST_ROOT_ROUTE = 'vault.cluster.secrets.backend.list-root';
const SHOW_ROUTE = 'vault.cluster.secrets.backend.show';

export default class TotpEdit extends Component {
  @service router;
  @service flashMessages;

  @tracked hasGenerated = false;
  @tracked invalidFormAlert = '';
  @tracked modelValidations;

  successCallback;

  get defaultKeyFormFields() {
    const shared = ['name', 'generate', 'issuer', 'accountName'];
    const generated = [...shared, 'exported'];
    const nonGenerated = [...shared, 'url', 'key'];
    return this.args.model.generate ? generated : nonGenerated;
  }

  get groups() {
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:
    this.router.transitionTo(...arguments);
  }

  @action
  reset() {
    this.model.unloadRecord();
    this.successCallback(null);
  }

  @action
  delete() {
    this.persist('destroyRecord', () => {
      this.transitionToRoute(LIST_ROOT_ROUTE);
    });
  }

  @action
  create(event) {
    event.preventDefault();
    const { isValid, state, invalidFormMessage } = this.model.validate();
    this.modelValidations = isValid ? null : state;
    this.invalidFormAlert = invalidFormMessage;
    if (isValid) {
      const modelId = this.model.name;

      // TODO verify url resolves for a key before submitting -> confusing error message
      this.persist('save', () => {
        this.transitionToRoute(SHOW_ROUTE, modelId);
      });
    }
  }
}


Review comment (suggestion):
I know this is WIP but eventually we'll want to make this method async. Going a step further, we typically make save methods (or create in this case) concurrency tasks so that we can leverage derived state like `mySaveTask.isRunning` to disable the save/cancel buttons in the form while the task is executing. [ember-concurrency docs](https://ember-concurrency.com/docs/derived-state)


Code after:
    this.args.model.unloadRecord();
    this.transitionToRoute(SHOW_ROUTE, name);
  }

  @action
  async deleteKey() {
    try {
      await this.args.model.destroyRecord();
      this.transitionToRoute(LIST_ROOT_ROUTE);
    } catch (err) {
      // err will display via model state
      return;
    }
    this.flashMessages.success(`${this.args.model.id} was successfully deleted.`);
  }

  createKey = task(
    waitFor(async (event) => {
      event.preventDefault();
      const { isValid, state, invalidFormMessage } = this.args.model.validate();
      this.modelValidations = isValid ? null : state;
      this.invalidFormAlert = invalidFormMessage;

      if (!isValid) return;
      try {
        const allFields = [...this.defaultKeyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;

        if (generate && exported) {
          // stay in this template and show QR code returned from response
          this.hasGenerated = true;
        } else {
          // nothing is returned from response, transition to key details route
          this.transitionToRoute(SHOW_ROUTE, this.args.model.name);
        }
      } catch (err) {
        // err will display via model state
        return;
      }
      this.flashMessages.success('Successfully created key.');
    })
  );
}
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:
  }

  @action
  reset() {
    this.model.unloadRecord();
    this.successCallback(null);
  }

  @action
  delete() {
    this.persist('destroyRecord', () => {
      this.transitionToRoute(LIST_ROOT_ROUTE);
    });
  }

  @action
  create(event) {
    event.preventDefault();
    const { isValid, state, invalidFormMessage } = this.model.validate();
    this.modelValidations = isValid ? null : state;
    this.invalidFormAlert = invalidFormMessage;
    if (isValid) {
      const modelId = this.model.name;

      // TODO verify url resolves for a key before submitting -> confusing error message
      this.persist('save', () => {
        this.transitionToRoute(SHOW_ROUTE, modelId);
      });
    }
  }
}


Review comment (refactor):

if the createRecord method is setup in the adapter correctly, you should be able to just call `this.mode.save()` here instead of the abstracted persist method


Code after:
    this.transitionToRoute(SHOW_ROUTE, name);
  }

  @action
  async deleteKey() {
    try {
      await this.args.model.destroyRecord();
      this.transitionToRoute(LIST_ROOT_ROUTE);
    } catch (err) {
      // err will display via model state
      return;
    }
    this.flashMessages.success(`${this.args.model.id} was successfully deleted.`);
  }

  createKey = task(
    waitFor(async (event) => {
      event.preventDefault();
      const { isValid, state, invalidFormMessage } = this.args.model.validate();
      this.modelValidations = isValid ? null : state;
      this.invalidFormAlert = invalidFormMessage;

      if (!isValid) return;
      try {
        const allFields = [...this.defaultKeyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;

        if (generate && exported) {
          // stay in this template and show QR code returned from response
          this.hasGenerated = true;
        } else {
          // nothing is returned from response, transition to key details route
          this.transitionToRoute(SHOW_ROUTE, this.args.model.name);
        }
      } catch (err) {
        // err will display via model state
        return;
      }
      this.flashMessages.success('Successfully created key.');
    })
  );
}
Repository: hashicorp/vault
File: ui/app/models/totp-key.js (JavaScript)

Code before:
    },
    {
      type: 'containsWhiteSpace',
      message:
        "Account name contains whitespace. If this is desired, you'll need to encode it with %20 in API requests.",
      level: 'warn',
    },
  ],
  issuer: [
    {
      validator(model) {
        const { generate, issuer } = model;
        // this is required when generate is true
        return generate && (!issuer || issuer.trim() === '') ? false : true;
      },
      message: "Issuer can't be blank when key provider is vault",
    },
  ],
  key: [
    {
      validator(model) {
        const { generate, key, url } = model;
        // this is required when generate is false and url is blank
        return !generate && (!url || url.trim() === '') && (!key || key.trim() === '') ? false : true;
      },
      message: "Key can't be blank when URL is empty",
    },
  ],
  key_size: [{ type: 'number', message: 'Key size must be a number.' }],
  name: [
    { type: 'presence', message: "Name can't be blank." },
    {
      type: 'containsWhiteSpace',
      message:
        "Name contains whitespace. If this is desired, you'll need to encode it with %20 in API requests.",
      level: 'warn',
    },
  ],
  qr_size: [{ type: 'number', message: 'QR size must be a number' }],
};

@withModelValidations(validations)
@withExpandedAttributes()
@withFormFields()
export default class TotpKeyModel extends Model {
  @attr('string', {
    readOnly: true,
  })
  backend;

  @attr('string', {

Review comment (suggestion):
```suggestion
      message: "Key can't be blank if key is being passed from another service and the URL is empty.",
```

Code after:
      level: 'warn',
    },
  ],
  issuer: [
    {
      validator(model) {
        const { generate, issuer } = model;
        // this is required when generate is true
        return generate && !isPresent(issuer) ? false : true;
      },
      message: "Issuer can't be blank when when the key is generated by Vault.",
    },
  ],
  key: [
    {
      validator(model) {
        const { generate, key, url } = model;
        // this is required when generate is false and url is blank
        return !generate && !isPresent(url) && !isPresent(key) ? false : true;
      },
      message: "Key can't be blank if key is being passed from another service and the URL is empty.",
    },
  ],
  keySize: [{ type: 'number', message: 'Key size must be a number.' }],
  name: [
    { type: 'presence', message: "Name can't be blank." },
    {
      type: 'containsWhiteSpace',
      message:
        "Name contains whitespace. If this is desired, you'll need to encode it with %20 in API requests.",
      level: 'warn',
    },
  ],
  qrSize: [{ type: 'number', message: 'QR size must be a number' }],
};

@withModelValidations(validations)
@withExpandedAttributes()
@withFormFields()
export default class TotpKeyModel extends Model {
  @attr('string', {
    readOnly: true,
  })
  backend;

  @attr('string', {
    subText: 'Specifies the name for this key.',
  })
  name;

  @attr('string', {
Repository: hashicorp/vault
File: ui/app/models/totp-key.js (JavaScript)

Code before:
        const { generate, issuer } = model;
        // this is required when generate is true
        return generate && (!issuer || issuer.trim() === '') ? false : true;
      },
      message: "Issuer can't be blank when key provider is vault",
    },
  ],
  key: [
    {
      validator(model) {
        const { generate, key, url } = model;
        // this is required when generate is false and url is blank
        return !generate && (!url || url.trim() === '') && (!key || key.trim() === '') ? false : true;
      },
      message: "Key can't be blank when URL is empty",
    },
  ],
  key_size: [{ type: 'number', message: 'Key size must be a number.' }],
  name: [
    { type: 'presence', message: "Name can't be blank." },
    {
      type: 'containsWhiteSpace',
      message:
        "Name contains whitespace. If this is desired, you'll need to encode it with %20 in API requests.",
      level: 'warn',
    },
  ],
  qr_size: [{ type: 'number', message: 'QR size must be a number' }],
};

@withModelValidations(validations)
@withExpandedAttributes()
@withFormFields()
export default class TotpKeyModel extends Model {
  @attr('string', {
    readOnly: true,
  })
  backend;

  @attr('string', {
    subText: 'Specifies the name for this key.',
  })
  name;

  @attr('string', {
    subText: `The name of the account associated with the key.`,
  })
  accountName;

  @attr('string', {
    possibleValues: ALGORITHMS,

Review comment (suggestion):
below, I don't think we need `withExpandedAttributes()`? since I think we're just using `withFormFields()`

Code after:
    },
  ],
  key: [
    {
      validator(model) {
        const { generate, key, url } = model;
        // this is required when generate is false and url is blank
        return !generate && !isPresent(url) && !isPresent(key) ? false : true;
      },
      message: "Key can't be blank if key is being passed from another service and the URL is empty.",
    },
  ],
  keySize: [{ type: 'number', message: 'Key size must be a number.' }],
  name: [
    { type: 'presence', message: "Name can't be blank." },
    {
      type: 'containsWhiteSpace',
      message:
        "Name contains whitespace. If this is desired, you'll need to encode it with %20 in API requests.",
      level: 'warn',
    },
  ],
  qrSize: [{ type: 'number', message: 'QR size must be a number' }],
};

@withModelValidations(validations)
@withExpandedAttributes()
@withFormFields()
export default class TotpKeyModel extends Model {
  @attr('string', {
    readOnly: true,
  })
  backend;

  @attr('string', {
    subText: 'Specifies the name for this key.',
  })
  name;

  @attr('string', {
    subText: 'The name of the account associated with the key. Required for keys generated by Vault.',
  })
  accountName;

  @attr('string', {
    possibleValues: ['SHA1', 'SHA256', 'SHA512'],
    defaultValue: 'SHA1',
  })
  algorithm;

  @attr('number', {
Repository: hashicorp/vault
File: ui/app/models/totp-key.js (JavaScript)

Code before:
  @attr('string', {
    readOnly: true,
  })
  barcode;

  get attrs() {
    const keys = ['accountName', 'name', 'algorithm', 'digits', 'issuer', 'period'];
    return keys.map((k) => this.allByKey[k]);
  }

  get generatedAttrs() {
    const keys = ['url'];
    return keys.map((k) => this.allByKey[k]);
  }

  get generateString() {
    return this.generate ? 'Vault' : 'Other service';
  }

  set generateString(value) {
    this.generate = value === 'Vault' ? true : false;
  }

  @lazyCapabilities(apiPath`${'backend'}/keys/${'id'}`, 'backend', 'id') keyPath;
  @alias('keyPath.canRead') canRead;
  @alias('keyPath.canDelete') canDelete;
  //TODO remove these aliases
}


Review comment (security):
I see there's a TODO below, but want to make sure in a follow on that you capture the updated pattern for this is. (there's actually a MORE updated pattern to use the [capabilities service](https://github.com/hashicorp/vault/blob/main/ui/lib/kv/addon/routes/secret.js#L15) - so that's what you should use in net new code. but since we're following the "generator" pattern, i think using the getters is okay here)
```js
  get canDelete() {
    return this.keyPath.get('canDelete');
  }
```

([code sample](https://github.com/hashicorp/vault/blob/main/ui/app/models/keymgmt/provider.js#L148-L152))

Code after:
  }

  get generatedAttrs() {
    const keys = ['url'];
    return keys.map((k) => this.allByKey[k]);
  }

  get generateString() {
    return this.generate ? 'Vault' : 'Other service';
  }

  set generateString(value) {
    this.generate = value === 'Vault' ? true : false;
  }

  @lazyCapabilities(apiPath`${'backend'}/keys/${'id'}`, 'backend', 'id') keyPath;

  get canDelete() {
    return this.keyPath.get('canDelete');
  }

  get canRead() {
    return this.keyPath.get('canRead');
  }
}