Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(HardwareDetails page): add CommitNavigationGraph to all tabs #610

Merged
merged 6 commits into from
Nov 28, 2024

Conversation

lfjnascimento
Copy link
Collaborator

@lfjnascimento lfjnascimento commented Nov 26, 2024

  • refactor: make CommitNavigationGraph route-agnostic
  • fix: commit history should be counted from the head backwards
  • feat: add filter by commit hash in hardwareDetails endpoint
  • feat: add CommitNavigationGraph to HardwareDetails tabs
  • feat: increase tree selection debounce to 2s
  • feat: the hardwareDetails endpoint only need a limit time

Close #477

How To test:

  1. Go to any HardwareDetails page
  2. Let just one three selected
  3. Navigate to the commit history using the CommitNavigationGraph

Note: the status count is not correct for the builds tab, this is because of the way that the endpoint commitHistory currently works. We have issue #601 to fix this, which will be resolved soon

@lfjnascimento lfjnascimento self-assigned this Nov 26, 2024
@WilsonNet
Copy link
Collaborator

please update the httpie request file with a request with a different head commit so we can now how the payload is supposed to be

Comment on lines 55 to 64
treeCommits: TTreeCommits,
): Record<string, string> => {
const selectedTrees: Record<string, string> = {};

selectedIndexes.forEach(idx => {
const key = idx.toString();
const value = treeCommits
? treeCommits[key] || TREE_SELECT_HEAD_VALUE
: TREE_SELECT_HEAD_VALUE;

selectedTrees[key] = value;
});

return selectedTrees;
Copy link
Collaborator

@WilsonNet WilsonNet Nov 27, 2024

Choose a reason for hiding this comment

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

Suggested change
treeCommits: TTreeCommits,
): Record<string, string> => {
const selectedTrees: Record<string, string> = {};
selectedIndexes.forEach(idx => {
const key = idx.toString();
const value = treeCommits
? treeCommits[key] || TREE_SELECT_HEAD_VALUE
: TREE_SELECT_HEAD_VALUE;
selectedTrees[key] = value;
});
return selectedTrees;
treeCommits: TTreeCommits = {},
): Record<string, string> => {
const selectedTrees: Record<string, string> = {};
selectedIndexes.forEach(idx => {
const key = idx.toString();
const value = treeCommits[idx] || TREE_SELECT_HEAD_VALUE;
selectedTrees[key] = value;
});
return selectedTrees;

@@ -24,6 +23,8 @@ import MemoizedIssuesList from '@/components/Cards/IssuesList';
import MemoizedHardwareTested from '@/components/Cards/HardwareTested';
import type { TestsTableFilter } from '@/types/tree/TreeDetails';

import TreeCommitNavigationGraph from '../TreeCommitNavigationGraph';
Copy link
Collaborator

Choose a reason for hiding this comment

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

relative import, we need the linter rule

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought we would use relative import when getting from the parent directory, I'll change it

@@ -26,6 +25,8 @@ import MemoizedIssuesList from '@/components/Cards/IssuesList';

import { DesktopGrid, InnerMobileGrid, MobileGrid } from '../TabGrid';

import TreeCommitNavigationGraph from '../TreeCommitNavigationGraph';
Copy link
Collaborator

Choose a reason for hiding this comment

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

relative import

@@ -27,6 +26,8 @@ import MemoizedHardwareTested from '@/components/Cards/HardwareTested';

import { TestsTable } from '@/components/TestsTable/TestsTable';

import TreeCommitNavigationGraph from '../TreeCommitNavigationGraph';
Copy link
Collaborator

Choose a reason for hiding this comment

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

relative import

@@ -35,9 +35,11 @@ import type { ArchCompilerStatus } from '@/types/general';

import FilterLink from '../../HardwareDetailsFilterLink';
import { MemoizedSummaryItem } from '../Build/BuildTab';
import HardwareCommitNavigationGraph from '../HardwareCommitNavigationGraph';
Copy link
Collaborator

Choose a reason for hiding this comment

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

relative import

Comment on lines 53 to 56
gitBranch={tree.gitRepositoryBranch || ''}
gitUrl={tree.gitRepositoryUrl || ''}
treeId={treeId || ''}
headCommitHash={tree.headGitCommitHash || ''}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't give fake news to the component, let the component deal with the undefined

diffFilter: TFilter;
gitUrl: string;
gitBranch: string;
headCommitHash: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make things that can be undefined optional here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice

@lfjnascimento lfjnascimento force-pushed the feat/HD_commit_graph2 branch 2 times, most recently from b6026fd to f990c39 Compare November 27, 2024 17:53
commit_hashes = [tree["git_commit_hash"] for tree in trees]
qFilter = Q(build__checkout__start_time__gte=start_datetime)
qFilter &= Q(build__checkout__start_time__lte=end_datetime)
qFilter |= Q(build__checkout__git_commit_hash__in=commit_hashes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if this comment went trough because it disappeared, but I don't think we need to filter with timestamps here since we already have the commit hashes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes I notice that and I'm removing it right now

Copy link
Collaborator

@WilsonNet WilsonNet left a comment

Choose a reason for hiding this comment

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

pre approving it

Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

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

There are some trees which seem to not be defaulted to its head, such as here

Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

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

Working well on my tests

@lfjnascimento lfjnascimento merged commit 79af132 into main Nov 28, 2024
5 checks passed
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.

Hardware detail history chart (only when single tree is selected)
3 participants