ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: ui/tests/acceptance/enterprise-namespaces-test.js (JavaScript)

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

import {
  click,
  settled,
  visit,
  fillIn,
  currentURL,
  waitFor,
  findAll,
  triggerKeyEvent,
  find,
} from '@ember/test-helpers';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { runCmd, createNS } from 'vault/tests/helpers/commands';
import { login, loginNs, logout } from 'vault/tests/helpers/auth/auth-helpers';
import { AUTH_FORM } from 'vault/tests/helpers/auth/auth-form-selectors';
import { GENERAL } from '../helpers/general-selectors';
import { NAMESPACE_PICKER_SELECTORS } from '../helpers/namespace-picker';

import sinon from 'sinon';

module('Acceptance | Enterprise | namespaces', function (hooks) {
  setupApplicationTest(hooks);

  let fetchSpy;

  hooks.beforeEach(() => {
    fetchSpy = sinon.spy(window, 'fetch');
    return login();
  });

  hooks.afterEach(() => {
    fetchSpy.restore();
  });

  test('it focuses the search input field when the component is loaded', async function (assert) {
    await click(NAMESPACE_PICKER_SELECTORS.toggle);

    // Verify that the search input field is focused
    const searchInput = find(NAMESPACE_PICKER_SELECTORS.searchInput);
    assert.strictEqual(
      document.activeElement,

Review comment (security):
Up to you, but I like to shorten then. For Secret Engine Selectors I used SES, or maybe here just NAMESPACE_PICKER. Shortening it saves the smallest amount of time when typing these out, those co-pilot kind of does it for you. Again, totally up to you.

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

import {
  click,
  settled,
  visit,
  fillIn,
  currentURL,
  findAll,
  triggerKeyEvent,
  find,
} from '@ember/test-helpers';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { runCmd, createNS, deleteNS } from 'vault/tests/helpers/commands';
import { login, loginNs, logout } from 'vault/tests/helpers/auth/auth-helpers';
import { AUTH_FORM } from 'vault/tests/helpers/auth/auth-form-selectors';
import { GENERAL } from '../helpers/general-selectors';
import { NAMESPACE_PICKER_SELECTORS } from '../helpers/namespace-picker';

import sinon from 'sinon';

async function createNamespaces(namespaces) {
  for (const ns of namespaces) {
    // Note: iterate through the namespace parts to create the full namespace path
    const parts = ns.split('/');
    let currentPath = '';

    for (const part of parts) {
      // Visit the parent namespace
      const url = `/vault/dashboard${currentPath && `?namespace=${currentPath.replaceAll('/', '%2F')}`}`;
      await visit(url);

      currentPath = currentPath ? `${currentPath}/${part}` : part;

      // Create the current namespace
      await runCmd(createNS(part), false);
      await settled();
    }

    // Reset to the root namespace
    const url = '/vault/dashboard';
    await visit(url);
  }
Repository: hashicorp/vault
File: ui/app/components/clients/date-range.ts (TypeScript)

Code before:
    const { name, value } = evt.target;
    if (name === 'end') {
      this.modalEnd = value;
    } else {
      this.modalStart = value;
    }
  }

  // used for CE date picker
  @action handleSave() {
    if (this.validationError) return;
    const params: OnChangeParams = {
      start_time: undefined,
      end_time: undefined,
    };
    this.startDate = this.modalStart;
    this.endDate = this.modalEnd;

    let year, month;
    [year, month] = this.startDate.split('-');
    if (year && month) {
      params.start_time = formatDateObject({ monthIdx: parseInt(month) - 1, year: parseInt(year) }, false);
    }
    [year, month] = this.endDate.split('-');
    if (year && month) {
      params.end_time = formatDateObject({ monthIdx: parseInt(month) - 1, year: parseInt(year) }, true);
    }

    this.args.onChange(params);
    this.onClose();
  }

  @action
  updateEnterpriseDateRange(start: string) {
    const params: OnChangeParams = {
      start_time: undefined,
      end_time: undefined,
    };

    const [year, month] = start.split('-');
    if (year && month) {
      // pass true for isEnd even for start because we want to go off last day of month here, otherwise we risk
      // setting it to a start_time that is for the previous billing period
      params.start_time = formatDateObject({ monthIdx: parseInt(month) - 1, year: parseInt(year) }, true);
    }

    this.args.onChange(params);
  }
}


Review comment (suggestion):
```suggestion
      params.end_time = formatDateObject({ monthIdx: parseInt(month, 10) - 1, year: parseInt(year, 10) }, true);
```

Code after:
    const { name, value } = evt.target;
    if (name === 'end') {
      this.modalEnd = value;
    } else {
      this.modalStart = value;
    }
  }

  // used for CE date picker
  @action handleSave() {
    if (this.validationError) return;
    const params: OnChangeParams = {
      start_time: undefined,
      end_time: undefined,
    };
    this.startDate = this.modalStart;
    this.endDate = this.modalEnd;

    let year, month;
    [year, month] = this.startDate.split('-');
    if (year && month) {
      params.start_time = formatDateObject({ monthIdx: Number(month) - 1, year: Number(year) }, false);
    }
    [year, month] = this.endDate.split('-');
    if (year && month) {
      params.end_time = formatDateObject({ monthIdx: Number(month) - 1, year: Number(year) }, true);
    }

    this.args.onChange(params);
    this.onClose();
  }

  @action
  updateEnterpriseDateRange(start: string) {
    const params: OnChangeParams = {
      start_time: undefined,
      end_time: undefined,
    };

    const [year, month] = start.split('-');
    if (year && month) {
      // pass true for isEnd even for start because we want to go off last day of month here, otherwise we risk
      // setting it to a start_time that is for the previous billing period
      params.start_time = formatDateObject({ monthIdx: parseInt(month) - 1, year: parseInt(year) }, true);
    }

    this.args.onChange(params);
  }
}
Repository: hashicorp/vault
File: ui/tests/integration/components/clients/date-range-test.js (JavaScript)

Code before:
    assert.false(this.onChange.called);

    await fillIn(DATE_RANGE.editDate('start'), '2018-01');
    await fillIn(DATE_RANGE.editDate('end'), '2017-05');
    assert.dom(DATE_RANGE.validation).hasText('Start date must be before end date.');
    await click(GENERAL.saveButton);
    assert.false(this.onChange.called);

    await click(GENERAL.cancelButton);
    assert.false(this.onChange.called);
    assert.dom(DATE_RANGE.editModal).doesNotExist();
  });

  test('it resets the tracked values on close', async function (assert) {
    await this.renderComponent();

    await click(DATE_RANGE.edit);
    await fillIn(DATE_RANGE.editDate('start'), '2017-04');
    await fillIn(DATE_RANGE.editDate('end'), '2018-05');
    await click(GENERAL.cancelButton);

    await click(DATE_RANGE.edit);
    assert.dom(DATE_RANGE.editDate('start')).hasValue('2018-01');
    assert.dom(DATE_RANGE.editDate('end')).hasValue('2019-01');
  });
});


Review comment (refactor):
So is the "disabled" state in the screenshot superficial and it's still selectable? Or does the `fillIn` helper just bypass the min/max settings of the date input 🤔 

Code after:
    await this.renderComponent();
    await click(DATE_RANGE.edit);
    await fillIn(DATE_RANGE.editDate('end'), '');
    assert.dom(DATE_RANGE.validation).hasText('You must supply both start and end dates.');
    await click(GENERAL.saveButton);
    assert.false(this.onChange.called);

    await fillIn(DATE_RANGE.editDate('start'), '2018-01');
    await fillIn(DATE_RANGE.editDate('end'), '2017-05');
    assert.dom(DATE_RANGE.validation).hasText('Start date must be before end date.');
    await click(GENERAL.saveButton);
    assert.false(this.onChange.called);

    await click(GENERAL.cancelButton);
    assert.false(this.onChange.called);
    assert.dom(DATE_RANGE.editModal).doesNotExist();
  });

  test('it does not allow the current month to be selected as a start date or as an end date', async function (assert) {
    this.owner.lookup('service:version').type = 'community';
    this.endTime = undefined;
    const currentMonth = format(timestamp.now(), 'yyyy-MM');

    await this.renderComponent();
    await click(DATE_RANGE.edit);
    await fillIn(DATE_RANGE.editDate('start'), currentMonth);
    await fillIn(DATE_RANGE.editDate('end'), currentMonth);

    assert.dom(DATE_RANGE.validation).hasText('You cannot select the current month or beyond.');
    await click(GENERAL.saveButton);
    assert.false(this.onChange.called);

    //  This tests validation when the end date is the current month and start is valid.
    //  If start is current month and end is a valid prior selection, it will run into the validation error of start being after end date
    //  which is covered by prior tests.
    await fillIn(DATE_RANGE.editDate('start'), '2018-01');
    await fillIn(DATE_RANGE.editDate('end'), currentMonth);
    await click(GENERAL.saveButton);
    assert.false(this.onChange.called);
  });
});
Repository: hashicorp/vault
File: ui/app/components/console/command-input.js (JavaScript)

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

import Component from '@ember/component';

export default Component.extend({
  onExecuteCommand() {},
  onFullscreen() {},
  onValueUpdate() {},
  onShiftCommand() {},
  value: null,
  isFullscreen: null,

  actions: {
    handleKeyUp(event) {
      const val = event.target.value;
      switch (event.key) {
        case 'Enter':
          this.onExecuteCommand(val);
          break;
        case 'ArrowUp':
        case 'ArrowDown':
          this.onShiftCommand(event.key);
          break;
        default:
          this.onValueUpdate(val);
      }
    },
    fullscreen() {
      this.onFullscreen();

Review comment (suggestion):
Instead of hardcoding the key values, please create a new enum file to replace the `key-codes.js` file that was deleted. example: [keyboard-keys.ts](https://github.com/hashicorp/vault/pull/30490/files)

Also, you'll want to add the `ui` label to this PR as well as the `1.20` milestone. 

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

import Component from '@ember/component';
import keys from 'core/utils/keys';

export default Component.extend({
  onExecuteCommand() {},
  onFullscreen() {},
  onValueUpdate() {},
  onShiftCommand() {},
  value: null,
  isFullscreen: null,

  actions: {
    handleKeyUp(event) {
      const val = event.target.value;
      switch (event.key) {
        case keys.ENTER:
          this.onExecuteCommand(val);
          break;
        case keys.UP:
        case keys.DOWN:
          this.onShiftCommand(event.key);
          break;
        default:
          this.onValueUpdate(val);
      }
    },
    fullscreen() {
Repository: hashicorp/vault
File: internalshared/configutil/merge_test.go (Go)

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

package configutil

import (
	"reflect"
	"testing"
	"time"

	"github.com/stretchr/testify/require"
)

func TestMerge(t *testing.T) {
	tests := []struct {
		name   string
		c1     *SharedConfig
		c2     *SharedConfig
		expect *SharedConfig
	}{
		{
			"nil second config",
			&SharedConfig{},
			nil,
			&SharedConfig{},
		},
		{
			"blank configs",
			&SharedConfig{},
			&SharedConfig{},
			&SharedConfig{},
		},
		{
			"combined FoundKeys",
			&SharedConfig{
				FoundKeys: []string{
					"DisableMlock",
					"ClusterName",
				},
			},

Review comment (suggestion):
This new test tests existing behavior. There are current fields that are still being dropped in the merge process. These include:
- `UnusedKeys`
- `Sections`
- `AdministrativeNamespacePath `

Not really sure what we want to do here. We could do some more testing to see what the expected behavior is for those fields. Initially I thought it would be good to have a test for `Merge` because there wasn't any, but as I look at this test, it's not going to uncover any issues aside from a regression in the function itself. As it's written, the test would not have uncovered the issue we had where merged configs were dropping the `FoundKeys` field. Open to feedback here.

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

package configutil

import (
	"reflect"
	"testing"
	"time"

	"github.com/stretchr/testify/require"
)

// TestMerge tests SharedConfig#Merge
func TestMerge(t *testing.T) {
	tests := []struct {
		name   string
		c1     *SharedConfig
		c2     *SharedConfig
		expect *SharedConfig
	}{
		{
			"nil second config",
			&SharedConfig{},
			nil,
			&SharedConfig{},
		},
		{
			"blank configs",
			&SharedConfig{},
			&SharedConfig{},
			&SharedConfig{},
		},
		{
			"combined FoundKeys",
			&SharedConfig{
				FoundKeys: []string{
					"DisableMlock",
					"ClusterName",
				},
Repository: hashicorp/vault
File: ui/app/components/clients/activity.ts (TypeScript)

Code before:
  get isCurrentMonth() {
    const { activity } = this.args;
    const current = parseAPITimestamp(activity.responseTimestamp) as Date;
    const start = parseAPITimestamp(activity.startTime) as Date;
    const end = parseAPITimestamp(activity.endTime) as Date;
    return isSameMonth(start, current) && isSameMonth(end, current);
  }

  get isDateRange() {
    const { activity } = this.args;
    return !isSameMonth(
      parseAPITimestamp(activity.startTime) as Date,
      parseAPITimestamp(activity.endTime) as Date
    );
  }

  // (object) top level TOTAL client counts for given date range
  get totalUsageCounts(): TotalClients {
    const { namespace, activity, mountPath } = this.args;
    // only do this if we have a mountPath filter.
    // namespace is filtered on API layer
    if (activity?.byMonth && namespace && mountPath) {
      return filteredTotalForMount(activity.byMonth, namespace, mountPath);
    }

    return activity.newClientTotal;
  }

  get upgradesDuringActivity() {
    const { versionHistory, activity } = this.args;
    return filterVersionHistory(versionHistory, activity.startTime, activity.endTime);
  }
}


Review comment (suggestion):
Per comment below, I believe this should be reverted because these usage counts should still map to the top-level `total` block for the date range. 

Code after:
    const start = parseAPITimestamp(activity.startTime) as Date;
    const end = parseAPITimestamp(activity.endTime) as Date;
    return isSameMonth(start, current) && isSameMonth(end, current);
  }

  get isDateRange() {
    const { activity } = this.args;
    return !isSameMonth(
      parseAPITimestamp(activity.startTime) as Date,
      parseAPITimestamp(activity.endTime) as Date
    );
  }

  // (object) top level TOTAL client counts for given date range
  get totalUsageCounts(): TotalClients {
    const { namespace, activity, mountPath } = this.args;
    // only do this if we have a mountPath filter.
    // namespace is filtered on API layer
    if (activity?.byNamespace && namespace && mountPath) {
      return filteredTotalForMount(activity.byNamespace, namespace, mountPath);
    }
    return activity?.total;
  }

  get upgradesDuringActivity() {
    const { versionHistory, activity } = this.args;
    return filterVersionHistory(versionHistory, activity.startTime, activity.endTime);
  }
}
Repository: hashicorp/vault
File: ui/app/components/clients/running-total.ts (TypeScript)

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

import Component from '@glimmer/component';
import type { TotalClients } from 'core/utils/client-count-utils';
import ClientsVersionHistoryModel from 'vault/vault/models/clients/version-history';
import { ByMonthNewClients } from '../../../lib/core/addon/utils/client-count-utils';

interface Args {
  isSecretsSyncActivated: boolean;
  byMonthNewClients: ByMonthNewClients[];
  isHistoricalMonth: boolean;
  isCurrentMonth: boolean;
  runningTotals: TotalClients;
  upgradesDuringActivity: ClientsVersionHistoryModel[];
  responseTimestamp: string;
  mountPath: string;
}

export default class RunningTotal extends Component<Args> {
  get chartContainerText() {
    const { isSecretsSyncActivated } = this.args;
    return `The total clients in the specified date range, displayed per month. This includes entity, non-entity${
      isSecretsSyncActivated ? ', ACME and secrets sync clients' : ' and ACME clients'
    }. The total client count number is an important consideration for Vault billing.`;
  }

  get runningTotalData() {
    return this.args.byMonthNewClients.map((monthly) => ({
      ...monthly,
      new_clients: monthly.clients,
    }));

Review comment (suggestion):
nit: this type lives in the same directory as TotalClients above, it looks like auto-import just got confused. I also went ahead and added `type` to the import on line 8 🙃 

```suggestion
import type { ByMonthNewClients, TotalClients } from 'core/utils/client-count-utils';
import type ClientsVersionHistoryModel from 'vault/vault/models/clients/version-history';
```

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

import Component from '@glimmer/component';
import type { ByMonthNewClients, TotalClients } from 'core/utils/client-count-utils';
import type ClientsVersionHistoryModel from 'vault/vault/models/clients/version-history';

interface Args {
  isSecretsSyncActivated: boolean;
  byMonthNewClients: ByMonthNewClients[];
  isHistoricalMonth: boolean;
  isCurrentMonth: boolean;
  runningTotals: TotalClients;
  upgradesDuringActivity: ClientsVersionHistoryModel[];
  responseTimestamp: string;
  mountPath: string;
}

export default class RunningTotal extends Component<Args> {
  get chartContainerText() {
    const { isSecretsSyncActivated } = this.args;
    return `The total clients in the specified date range, displayed per month. This includes entity, non-entity${
      isSecretsSyncActivated ? ', ACME and secrets sync clients' : ' and ACME clients'
    }. The total client count number is an important consideration for Vault billing.`;
  }

  get runningTotalData() {
    return this.args.byMonthNewClients.map((monthly) => ({
      ...monthly,
      new_clients: monthly.clients,
    }));
  }
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes/login-settings/rule/details.js (JavaScript)

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service store;

  async model() {
    try {
      const { name } = this.paramsFor('login-settings.rule');
      if (!name) return null;

      const adapter = this.store.adapterFor('application');
      const rule = await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(name)}`, 'GET');

      return { rule: rule.data };
    } catch (e) {
      return null;
    }
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: '' }];
  }
}


Review comment (security):
we don't want to return null here, but instead return the actual error which should bubble up and render the appropriate error template (it could be a permissions or not found error, for example). To test this is working correctly, try navigating to a rule in the URL that doesn't exist in the mirage DB. 

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service('app-router') router;
  @service store;

  beforeModel() {
    const { name } = this.paramsFor('login-settings.rule');
    if (!name) {
      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    }
  }

  async model() {
    const { name } = this.paramsFor('login-settings.rule');

    const adapter = this.store.adapterFor('application');
    const rule = await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(name)}`, 'GET');

    return { rule: rule.data };
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: '' }];
  }
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes/login-settings/rule/details.js (JavaScript)

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service store;

  async model() {
    try {
      const { name } = this.paramsFor('login-settings.rule');
      if (!name) return null;

      const adapter = this.store.adapterFor('application');
      const rule = await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(name)}`, 'GET');

      return { rule: rule.data };
    } catch (e) {
      return null;
    }
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: '' }];
  }
}


Review comment (suggestion):
Returning null would then mean there's no data returned to this route. If there's no name param we should handle this in the `beforeModel` hook and transition them to the list view instead

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service('app-router') router;
  @service store;

  beforeModel() {
    const { name } = this.paramsFor('login-settings.rule');
    if (!name) {
      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    }
  }

  async model() {
    const { name } = this.paramsFor('login-settings.rule');

    const adapter = this.store.adapterFor('application');
    const rule = await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(name)}`, 'GET');

    return { rule: rule.data };
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: '' }];
  }
}