ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: api/sys_utilization_report.go (Go)

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

package api

import (
	"context"
	"errors"
	"net/http"

	"github.com/mitchellh/mapstructure"
)

func (c *Sys) UtilizationReport() (*UtilizationReportOutput, error) {
	return c.UtilizationReportWithContext(context.Background())
}

func (c *Sys) UtilizationReportWithContext(ctx context.Context) (*UtilizationReportOutput, error) {
	ctx, cancelFunc := c.c.withConfiguredTimeout(ctx)
	defer cancelFunc()

	r := c.c.NewRequest(http.MethodGet, "/v1/sys/utilization-report")

	resp, err := c.c.rawRequestWithContext(ctx, r)
	if err != nil {
		return nil, err
	}
	defer resp.Body.Close()

	secret, err := ParseSecret(resp.Body)
	if err != nil {
		return nil, err
	}
	if secret == nil || secret.Data == nil {
		return nil, errors.New("data from server response is empty")
	}

	var result UtilizationReportOutput
	err = mapstructure.Decode(secret.Data, &result)
	if err != nil {
		return nil, err
	}


Review comment (suggestion):
Broadly speaking, this looks good, but we should update the client in lockstep with the changes we make to support the, to avoid having an API package (even temporarily) that lacks compatibility with the API itself.

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

package api

import (
	"context"
	"errors"
	"net/http"

	"github.com/mitchellh/mapstructure"
)

func (c *Sys) UtilizationReport() (*UtilizationReportOutput, error) {
	return c.UtilizationReportWithContext(context.Background(), "")
}

func (c *Sys) UtilizationReportWithNamespace(nsPath string) (*UtilizationReportOutput, error) {
	return c.UtilizationReportWithContext(context.Background(), nsPath)
}

func (c *Sys) UtilizationReportWithContext(ctx context.Context, nsPath string) (*UtilizationReportOutput, error) {
	ctx, cancelFunc := c.c.withConfiguredTimeout(ctx)
	defer cancelFunc()

	r := c.c.NewRequest(http.MethodGet, "/v1/sys/utilization-report")

	if nsPath != "" {
		r.Params.Add("namespace", nsPath)
	}

	resp, err := c.c.rawRequestWithContext(ctx, r)
	if err != nil {
		return nil, err
	}
	defer resp.Body.Close()

	secret, err := ParseSecret(resp.Body)
	if err != nil {
		return nil, err
	}
	if secret == nil || secret.Data == nil {
		return nil, errors.New("data from server response is empty")
Repository: hashicorp/vault
File: ui/app/components/sidebar/nav/cluster.js (JavaScript)

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

import Component from '@glimmer/component';
import { service } from '@ember/service';

export default class SidebarNavClusterComponent extends Component {
  @service currentCluster;
  @service flags;
  @service version;
  @service auth;
  @service namespace;
  @service permissions;

  get cluster() {
    return this.currentCluster.cluster;
  }

  get hasChrootNamespace() {
    return this.cluster?.hasChrootNamespace;
  }

  get isRootNamespace() {
    // should only return true if we're in the true root namespace
    return this.namespace.inRootNamespace && !this.hasChrootNamespace;
  }

  get canAccessVaultUsageDashboard() {
    /*
    A user can access Vault Usage if they satisfy the following conditions:
      1) They have access to sys/v1/utilization-report endpoint

Review comment (refactor):
Oh cool, so this acts as a sort of development feature flag. Good idea!

(This can be in a separate PR) but this could be formalized a bit and drawn into a helper so you don't need the getter _every_ time.

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

import Component from '@glimmer/component';
import { service } from '@ember/service';
import config from 'vault/config/environment';

export default class SidebarNavClusterComponent extends Component {
  @service currentCluster;
  @service flags;
  @service version;
  @service auth;
  @service namespace;
  @service permissions;

  get cluster() {
    return this.currentCluster.cluster;
  }

  get hasChrootNamespace() {
    return this.cluster?.hasChrootNamespace;
  }

  get isRootNamespace() {
    // should only return true if we're in the true root namespace
    return this.namespace.inRootNamespace && !this.hasChrootNamespace;
  }

  get canAccessVaultUsageDashboard() {
    /*
    A user can access Vault Usage if they satisfy the following conditions:
Repository: hashicorp/vault
File: ui/app/routes/vault/cluster/recovery/snapshots.ts (TypeScript)

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

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

import { SystemListStorageRaftSnapshotLoadListEnum } from '@hashicorp/vault-client-typescript';

import type ApiService from 'vault/services/api';
import type Capabilities from 'vault/services/capabilities';

export default class RecoverySnapshotsRoute extends Route {
  @service declare readonly api: ApiService;
  @service declare readonly capabilities: Capabilities;

  async model() {
    const path = 'sys/storage/raft/snapshot/snapshot-load';
    const capabilities = await this.capabilities.fetch([path]);

    const canLoadSnapshot = capabilities[path]?.canUpdate;

    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );

      const snapshots = await Promise.all(
        (keys ?? []).map((key: string) => this.api.sys.systemReadStorageRaftSnapshotLoadId(key))
      );

      return hash({
        snapshots,
        canLoadSnapshot,
      });
    } catch (e) {
      // return empty list of snapshots
      return hash({
        snapshots: [],
        canLoadSnapshot,
      });
    }
  }
}


Review comment (security):
We only want to catch if the error is a 404, if the person doesn't have permission to list snapshots then we want to surface that error

Code after:
      'sys/storage/raft/snapshot/snapshot-load'
    );

    const snapshots = await this.fetchSnapshots();

    return {
      snapshots,
      canLoadSnapshot: canUpdate,
    };
  }

  afterModel(model: SnapshotRouteModel) {
    if (model.snapshots.length === 1) {
      const snapshot_id = model.snapshots[0];
      this.router.transitionTo('vault.cluster.recovery.snapshots.snapshot.manage', snapshot_id);
    }
  }

  // todo: If this req fails bc user cannot list snapshots, they cannot use the UI (confirm with product)
  async fetchSnapshots() {
    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );
      return keys as string[];
    } catch (e) {
      const error = await this.api.parseError(e);
      if (error.status === 404) {
        return [];
      }
      throw {
        httpStatus: error.status,
        ...error,
      };
    }
  }
}
Repository: hashicorp/vault
File: ui/app/routes/vault/cluster/recovery/snapshots.ts (TypeScript)

Code before:

import { SystemListStorageRaftSnapshotLoadListEnum } from '@hashicorp/vault-client-typescript';

import type ApiService from 'vault/services/api';
import type Capabilities from 'vault/services/capabilities';

export default class RecoverySnapshotsRoute extends Route {
  @service declare readonly api: ApiService;
  @service declare readonly capabilities: Capabilities;

  async model() {
    const path = 'sys/storage/raft/snapshot/snapshot-load';
    const capabilities = await this.capabilities.fetch([path]);

    const canLoadSnapshot = capabilities[path]?.canUpdate;

    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );

      const snapshots = await Promise.all(
        (keys ?? []).map((key: string) => this.api.sys.systemReadStorageRaftSnapshotLoadId(key))
      );

      return hash({
        snapshots,
        canLoadSnapshot,
      });
    } catch (e) {
      // return empty list of snapshots
      return hash({
        snapshots: [],
        canLoadSnapshot,
      });
    }
  }
}


Review comment (suggestion):
hash waits to return until all promises are resolved, so there's no need to use it here since both the variables are awaited above

Code after:

import type ApiService from 'vault/services/api';
import type Capabilities from 'vault/services/capabilities';
import type RouterService from '@ember/routing/router-service';
import type { ModelFrom } from 'vault/vault/route';

type SnapshotRouteModel = ModelFrom<RecoverySnapshotsRoute>;

export default class RecoverySnapshotsRoute extends Route {
  @service declare readonly api: ApiService;
  @service declare readonly capabilities: Capabilities;
  @service declare readonly router: RouterService;

  async model() {
    const { canUpdate } = await this.capabilities.fetchPathCapabilities(
      'sys/storage/raft/snapshot/snapshot-load'
    );

    const snapshots = await this.fetchSnapshots();

    return {
      snapshots,
      canLoadSnapshot: canUpdate,
    };
  }

  afterModel(model: SnapshotRouteModel) {
    if (model.snapshots.length === 1) {
      const snapshot_id = model.snapshots[0];
      this.router.transitionTo('vault.cluster.recovery.snapshots.snapshot.manage', snapshot_id);
    }
  }

  // todo: If this req fails bc user cannot list snapshots, they cannot use the UI (confirm with product)
  async fetchSnapshots() {
    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );
      return keys as string[];
    } catch (e) {
      const error = await this.api.parseError(e);
      if (error.status === 404) {
        return [];
      }
      throw {
        httpStatus: error.status,
        ...error,
      };
    }
  }
Repository: hashicorp/vault
File: ui/app/routes/vault/cluster/recovery/snapshots.ts (TypeScript)

Code before:
export default class RecoverySnapshotsRoute extends Route {
  @service declare readonly api: ApiService;
  @service declare readonly capabilities: Capabilities;
  @service declare readonly router: RouterService;

  async model() {
    const { canUpdate } = await this.capabilities.fetchPathCapabilities(
      'sys/storage/raft/snapshot/snapshot-load'
    );

    const snapshots = await this.fetchSnapshots();

    return {
      snapshots,
      canLoadSnapshot: canUpdate,
    };
  }

  afterModel(model: SnapshotRouteModel) {
    if (model.snapshots.length === 1) {
      const snapshot_id = model.snapshots[0];
      this.router.transitionTo('vault.cluster.recovery.snapshots.snapshot.manage', snapshot_id);
    }
  }

  // todo: If this req fails bc user cannot list snapshots, they cannot use the UI (confirm with product)
  async fetchSnapshots() {
    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );
      return keys as string[];
    } catch (e) {
      const { message, status } = await this.api.parseError(e);
      if (status === 404) {
        return [];
      }
      throw message;
    }
  }
}


Review comment (security):
Should we ask about snapshot `LIST` permissions in the project channel and how the UI should handle it? That way we can make a ticket, but I would consider that lower priority.

Code after:
export default class RecoverySnapshotsRoute extends Route {
  @service declare readonly api: ApiService;
  @service declare readonly capabilities: Capabilities;
  @service declare readonly router: RouterService;

  async model() {
    const { canUpdate } = await this.capabilities.fetchPathCapabilities(
      'sys/storage/raft/snapshot/snapshot-load'
    );

    const snapshots = await this.fetchSnapshots();

    return {
      snapshots,
      canLoadSnapshot: canUpdate,
    };
  }

  afterModel(model: SnapshotRouteModel) {
    if (model.snapshots.length === 1) {
      const snapshot_id = model.snapshots[0];
      this.router.transitionTo('vault.cluster.recovery.snapshots.snapshot.manage', snapshot_id);
    }
  }

  // todo: If this req fails bc user cannot list snapshots, they cannot use the UI (confirm with product)
  async fetchSnapshots() {
    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );
      return keys as string[];
    } catch (e) {
      const error = await this.api.parseError(e);
      if (error.status === 404) {
        return [];
      }
      throw {
        httpStatus: error.status,
        ...error,
      };
    }
  }
}
Repository: hashicorp/vault
File: ui/app/routes/vault/cluster/recovery/snapshots/snapshot.ts (TypeScript)

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

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

import type ApiService from 'vault/services/api';

export default class RecoverySnapshotsSnapshotRoute extends Route {
  @service declare readonly api: ApiService;
  model(params: { snapshot_id: string }) {
    return this.api.sys.systemReadStorageRaftSnapshotLoadId(params.snapshot_id);
  }
}


Review comment (suggestion):
nit: 
```suggestion
  @service declare readonly api: ApiService;
  
  model(params: { snapshot_id: string }) {
```

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

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

import type ApiService from 'vault/services/api';

export default class RecoverySnapshotsSnapshotRoute extends Route {
  @service declare readonly api: ApiService;

  model(params: { snapshot_id: string }) {
    return this.api.sys.systemReadStorageRaftSnapshotLoadId(params.snapshot_id);
  }
}
Repository: hashicorp/vault
File: ui/app/routes/vault/cluster/recovery/snapshots.ts (TypeScript)

Code before:
    return {
      snapshots,
      canLoadSnapshot: canUpdate,
    };
  }

  afterModel(model: SnapshotRouteModel) {
    if (model.snapshots.length === 1) {
      const snapshot_id = model.snapshots[0];
      this.router.transitionTo('vault.cluster.recovery.snapshots.snapshot.manage', snapshot_id);
    }
  }

  // todo: If this req fails bc user cannot list snapshots, they cannot use the UI (confirm with product)
  async fetchSnapshots() {
    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );
      return keys as string[];
    } catch (e) {
      const { message, status } = await this.api.parseError(e);
      if (status === 404) {
        return [];
      }
      throw message;
    }
  }
}


Review comment (security):
I think we actually just need to `throw e` here so the error bubbles up to the error template correctly. Here's an [example](https://github.com/hashicorp/vault/blob/main/ui/app/routes/vault/cluster/secrets/backend.js#L27-L32). 

```suggestion
      const error = await this.api.parseError(e);
      if (error.status === 404) {
        return [];
      }
      throw {
        httpStatus: error.status,
        ...error,
      };
```

Code after:
    return {
      snapshots,
      canLoadSnapshot: canUpdate,
    };
  }

  afterModel(model: SnapshotRouteModel) {
    if (model.snapshots.length === 1) {
      const snapshot_id = model.snapshots[0];
      this.router.transitionTo('vault.cluster.recovery.snapshots.snapshot.manage', snapshot_id);
    }
  }

  // todo: If this req fails bc user cannot list snapshots, they cannot use the UI (confirm with product)
  async fetchSnapshots() {
    try {
      const { keys } = await this.api.sys.systemListStorageRaftSnapshotLoad(
        SystemListStorageRaftSnapshotLoadListEnum.TRUE
      );
      return keys as string[];
    } catch (e) {
      const error = await this.api.parseError(e);
      if (error.status === 404) {
        return [];
      }
      throw {
        httpStatus: error.status,
        ...error,
      };
    }
  }
}
Repository: hashicorp/vault
File: ui/tests/acceptance/recovery/snapshots-test.js (JavaScript)

Code before:

    this.store = this.owner.lookup('service:store');
    return login();
  });

  test('it renders empty state when no snapshots are loaded', async function (assert) {
    this.server.get('/sys/storage/raft/snapshot-load', () => {
      return new Response(404, { 'Content-Type': 'application/json' }, JSON.stringify({ errors: [] }));
    });

    await visit('/vault/recovery/snapshots');

    assert.strictEqual(currentURL(), '/vault/recovery/snapshots');

    assert.dom(GENERAL.emptyStateTitle).hasText('Upload a snapshot to get started');
    assert.dom(GENERAL.emptyStateActions).hasText('Upload snapshot');
  });

  test('it redirects to snapshot route when a snapshot is loaded', async function (assert) {
    this.server.get('/sys/storage/raft/snapshot-load', () =>
      overrideResponse(null, {
        data: {
          keys: ['1234'],
        },
      })
    );

    this.server.get('/sys/storage/raft/snapshot-load/1234', () =>
      overrideResponse(null, {
        data: {
          status: 'ready',
          expires_at: new Date(),
          snapshot_id: '1234',
        },
      })
    );

    await visit('vault/recovery/snapshots');

    assert.strictEqual(currentURL(), '/vault/recovery/snapshots/1234/manage');
    assert.strictEqual(currentRouteName(), 'vault.cluster.recovery.snapshots.snapshot.manage');
  });
});


Review comment (suggestion):
You don't need `overrideResponse` here, that's just a convenience function to stub the response shape - which for some reason didn't work above!
```suggestion
    this.server.get('/sys/storage/raft/snapshot-load', () => {
    return { data: { keys: ['1234'] } }
  })
```

Code after:
    return login();
  });

  test('it renders empty state when no snapshots are loaded', async function (assert) {
    this.server.get('/sys/storage/raft/snapshot-load', () => {
      return new Response(404, { 'Content-Type': 'application/json' }, JSON.stringify({ errors: [] }));
    });

    await visit('/vault/recovery/snapshots');

    assert.strictEqual(currentURL(), '/vault/recovery/snapshots');

    assert.dom(GENERAL.emptyStateTitle).hasText('Upload a snapshot to get started');
    assert.dom(GENERAL.emptyStateActions).hasText('Upload snapshot');
  });

  test('it redirects to snapshot route when a snapshot is loaded', async function (assert) {
    this.server.get('/sys/storage/raft/snapshot-load', () => {
      return { data: { keys: ['1234'] } };
    });

    this.server.get('/sys/storage/raft/snapshot-load/1234', () => {
      return {
        data: {
          status: 'ready',
          expires_at: new Date(),
          snapshot_id: '1234',
        },
      };
    });

    await visit('vault/recovery/snapshots');

    assert.strictEqual(currentURL(), '/vault/recovery/snapshots/1234/manage');
    assert.strictEqual(currentRouteName(), 'vault.cluster.recovery.snapshots.snapshot.manage');
  });
});
Repository: hashicorp/vault
File: ui/tests/integration/components/recovery/page/snapshots-test.js (JavaScript)

Code before:
    this.model.canLoadSnapshot = true;
    await render(hbs`<Recovery::Page::Snapshots @model={{this.model}}/>`);
    assert
      .dom(GENERAL.emptyStateTitle)
      .hasText('Upload a snapshot to get started', 'empty state title renders');
    assert
      .dom(GENERAL.emptyStateMessage)
      .hasText(
        'Secrets Recovery allows you to restore accidentally deleted or lost secrets from a snapshot. The snapshots can be provided via upload or loaded from external storage.',
        'empty state message renders'
      );
    assert.dom(GENERAL.emptyStateActions).hasText('Upload snapshot', 'empty state action renders');
  });

  test('it displays loaded snapshot card', async function (assert) {
    const expiryDate = dateFormat(
      [new Date('2023-09-20T10:51:53.961861096-04:00'), 'MMMM do yyyy, h:mm:ss a'],
      {}
    );

    this.model = {
      snapshot_id: '6ecc06a9-3592-26f9-40cb-8d1b511890a6',
      status: 'ready',
      expires_at: expiryDate,
    };

    await render(hbs`<Recovery::Page::Snapshots::SnapshotManage @model={{this.model}}/>`);

    assert.dom(SELECTORS.badge('status')).hasText('ready', 'status badge renders');
  });
});


Review comment (suggestion):
This test file is for the `Recovery::Page::Snapshots @model={{this.model}}/>` component so this belongs in a test file for the SnapshotsManage component. 

That's fine if you want to leave for now and we can move it over when we finish building out that view, but generally it's good practice to only test one component per module/test file

Code after:
    assert
      .dom(GENERAL.emptyStateMessage)
      .hasText(
        'Secrets Recovery allows you to restore accidentally deleted or lost secrets from a snapshot. The snapshots can be provided via upload or loaded from external storage.',
        'empty state message renders'
      );
    assert.dom(GENERAL.emptyStateActions).hasText('Upload snapshot', 'empty state action renders');
  });

  // TODO will move over to separate test file with further feature work
  test('it displays loaded snapshot card', async function (assert) {
    const expiryDate = dateFormat(
      [new Date('2023-09-20T10:51:53.961861096-04:00'), 'MMMM do yyyy, h:mm:ss a'],
      {}
    );

    this.model = {
      snapshot_id: '6ecc06a9-3592-26f9-40cb-8d1b511890a6',
      status: 'ready',
      expires_at: expiryDate,
    };

    await render(hbs`<Recovery::Page::Snapshots::SnapshotManage @model={{this.model}}/>`);

    assert.dom(SELECTORS.badge('status')).hasText('ready', 'status badge renders');
  });
});