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

Code before:
  @attr('number', {
    possibleValues: SKEW,
    defaultValue: 1,
  })
  skew;

  @attr('boolean', {
    editType: 'toggleButton',
    defaultValue: true,
    helperTextDisabled: 'Vault will not return QR code and url upon key creation.',
    helperTextEnabled: 'QR code and URL will be returned upon generating a key.',
  })
  exported;

  @attr('number', {
    label: 'QR size',
    defaultValue: 0,
  })
  qrSize;

  // Used when generate is false
  @attr('string', {
    label: 'URL',
    helpText:
      'If a URL is provided the other fields can be left empty. E.g. otpauth://totp/Vault:test@test.com?secret=Y64VEVMBTSXCYIWRSHRNDZW62MPGVU2G&issuer=Vault',
    subText: `The TOTP key url string that can be used to configure a key.`,
  })
  url;

  @attr('string', {
    label: 'Key',
    subText: `The root key used to generate a TOTP code.`,
  })
  key;

  // Returned when a key is created as provider
  @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]);
  }


Review comment (suggestion):
total nit but these can be single quotes since there's no apostrophes in the string
```suggestion
    subText: 'The TOTP key url string that can be used to configure a key.',
```

Code after:
    editType: 'toggleButton',
    defaultValue: true,
    helperTextDisabled: 'Vault will not return QR code and url upon key creation.',
    helperTextEnabled: 'QR code and URL will be returned upon generating a key.',
  })
  exported;

  @attr('number', {
    label: 'QR size',
    defaultValue: 0,
  })
  qrSize;

  // Used when generate is false
  @attr('string', {
    label: 'URL',
    helpText:
      'If a URL is provided the other fields can be left empty. E.g. otpauth://totp/Vault:test@test.com?secret=Y64VEVMBTSXCYIWRSHRNDZW62MPGVU2G&issuer=Vault',
    subText: 'The TOTP key url string that can be used to configure a key.',
  })
  url;

  @attr('string', {
    subText: 'The root key used to generate a TOTP code.',
  })
  key;

  // Returned when a key is created as provider
  @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;
  }
Repository: hashicorp/vault
File: ui/app/models/totp-key.js (JavaScript)

Code before:
  @attr('boolean', {
    editType: 'toggleButton',
    defaultValue: true,
    helperTextDisabled: 'Vault will not return QR code and url upon key creation.',
    helperTextEnabled: 'QR code and URL will be returned upon generating a key.',
  })
  exported;

  @attr('number', {
    label: 'QR size',
    defaultValue: 0,
  })
  qrSize;

  // Used when generate is false
  @attr('string', {
    label: 'URL',
    helpText:
      'If a URL is provided the other fields can be left empty. E.g. otpauth://totp/Vault:test@test.com?secret=Y64VEVMBTSXCYIWRSHRNDZW62MPGVU2G&issuer=Vault',
    subText: `The TOTP key url string that can be used to configure a key.`,
  })
  url;

  @attr('string', {
    label: 'Key',
    subText: `The root key used to generate a TOTP code.`,
  })
  key;

  // Returned when a key is created as provider
  @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;

Review comment (suggestion):
nit: [form field](https://github.com/hashicorp/vault/blob/main/ui/lib/core/addon/components/form-field.js#L132-L141) automatically capitalizes the attribute name, so unless we need to change that behavior (like above, we want the label to read `URL` and not `Url`), there's no need to pass a "label" key
```suggestion
    subText: 'The root key used to generate a TOTP code.',
```

Code after:

  @attr('number', {
    label: 'QR size',
    defaultValue: 0,
  })
  qrSize;

  // Used when generate is false
  @attr('string', {
    label: 'URL',
    helpText:
      'If a URL is provided the other fields can be left empty. E.g. otpauth://totp/Vault:test@test.com?secret=Y64VEVMBTSXCYIWRSHRNDZW62MPGVU2G&issuer=Vault',
    subText: 'The TOTP key url string that can be used to configure a key.',
  })
  url;

  @attr('string', {
    subText: 'The root key used to generate a TOTP code.',
  })
  key;

  // Returned when a key is created as provider
  @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;

  get canDelete() {
    return this.keyPath.get('canDelete');
  }
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:

    if (generate) {
      groups['Provider Options'] = ['keySize', 'skew', 'qrSize'];
    }

    return groups;
  }

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

  transitionToRoute() {
    this.router.transitionTo(...arguments);
  }

  @action
  reset() {
    this.args.model.unloadRecord();
    this.transitionToRoute(SHOW_ROUTE, this.args.model.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('Successfully deleted key');
  }

  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.keyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;
        if (generate && exported) this.hasGenerated = true;

Review comment (suggestion):
I'm not sure this is true for `destroyMethod` - I'd have to dig in the docs. Older patterns in the codebase relied on the model's errors state but we've moved away from that. It's likely we should explicitly track `errorMessage` here and set accordingly.

Code after:

    if (generate) {
      groups['Provider Options'] = ['keySize', 'skew', 'qrSize'];
    }

    return groups;
  }

  transitionToRoute() {
    this.router.transitionTo(...arguments);
  }

  @action
  reset() {
    const { name } = this.args.model;
    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;
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';
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 or view the QR code of the 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 keyFormFields() {
    const shared = ['name', 'generate', 'issuer', 'accountName'];
    const generated = [...shared, 'exported'];
    const nonGenerated = [...shared, 'url', 'key'];
    return this.args.model.generate ? generated : nonGenerated;

Review comment (suggestion):
Since we were confused about this earlier this week, adding the generate functionality might be helpful here
```suggestion
 * `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.
```

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;
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:

  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 groups() {
    const { generate } = this.args.model;

    const groups = {
      'TOTP Code Options': ['algorithm', 'digits', 'period'],
    };

    if (generate) {
      groups['Provider Options'] = ['keySize', 'skew', 'qrSize'];
    }

    return groups;
  }

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

  transitionToRoute() {
    this.router.transitionTo(...arguments);
  }

  @action
  reset() {
    this.args.model.unloadRecord();
    this.transitionToRoute(SHOW_ROUTE, this.args.model.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('Successfully deleted key');
  }

  createKey = task(

Review comment (refactor):
It doesn't look like we reference `this.mode` anywhere - all references are directly for the arg `@mode` from what I can see. Can we remove this?

Code after:

  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() {
    const { generate } = this.args.model;

    const groups = {
      'TOTP Code Options': ['algorithm', 'digits', 'period'],
    };

    if (generate) {
      groups['Provider Options'] = ['keySize', 'skew', 'qrSize'];
    }

    return groups;
  }

  transitionToRoute() {
    this.router.transitionTo(...arguments);
  }

  @action
  reset() {
    const { name } = this.args.model;
    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();
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:
  }

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

  transitionToRoute() {
    this.router.transitionTo(...arguments);
  }

  @action
  reset() {
    this.args.model.unloadRecord();
    this.transitionToRoute(SHOW_ROUTE, this.args.model.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('Successfully deleted key');
  }

  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.keyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;
        if (generate && exported) this.hasGenerated = true;
        else this.reset();
      } catch (err) {
        // err will display via model state
        return;
      }
      this.flashMessages.success('Successfully created key');

Review comment (suggestion):
```suggestion
    this.flashMessages.success('Successfully deleted key.');
```

Code after:
  }

  transitionToRoute() {
    this.router.transitionTo(...arguments);
  }

  @action
  reset() {
    const { name } = this.args.model;
    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
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:
    this.args.model.unloadRecord();
    this.transitionToRoute(SHOW_ROUTE, this.args.model.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('Successfully deleted key');
  }

  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.keyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;
        if (generate && exported) this.hasGenerated = true;
        else this.reset();
      } catch (err) {
        // err will display via model state
        return;
      }
      this.flashMessages.success('Successfully created key');
    })
  );
}


Review comment (suggestion):
usually we refer to fields outside toggle groups as "default" fields. I wonder if renaming the keyFormFields would make that distinction clearer. Not a big deal! so fine to leave as is
```suggestion
        const allFields = [...this.defaultKeyFormFields, ...Object.values(this.groups).flat()];
```

Code after:

  @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:
      this.transitionToRoute(LIST_ROOT_ROUTE);
    } catch (err) {
      // err will display via model state
      return;
    }
    this.flashMessages.success('Successfully deleted key');
  }

  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.keyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;
        if (generate && exported) this.hasGenerated = true;
        else this.reset();
      } catch (err) {
        // err will display via model state
        return;
      }
      this.flashMessages.success('Successfully created key');
    })
  );
}


Review comment (bug):
Great job addressing the generated vs not generated logic. 

I think we should avoid unloading the record here, there are some challenging bugs to track down because we manually unload records and then try to access properties on them. As I mentioned above there have been some improvements to ember data, so I think manually unloading records is something we can move away from. 

I also think the block conditional here makes it easier to discern these are mutually exclusive
```suggestion
        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);
        }
```

Code after:
      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.');
    })
  );
}