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: Team plan repos list #2357

Merged
merged 28 commits into from
Nov 3, 2023
Merged

feat: Team plan repos list #2357

merged 28 commits into from
Nov 3, 2023

Conversation

RulaKhaled
Copy link
Contributor

Description

Introducing the new table for the team plan, with the following features different:

  • Use the new table UI
  • Hide the test coverage column
  • Support internal sorting

Notable Changes

  • Taking out shared components between old repose table and the new one to the repos list with some necessary refactors
  • Adjust repos list to accommodate the new team plan table
  • the new table!
  • tests

Screenshots

Screen.Recording.2023-10-30.at.10.58.21.AM.mov

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@netlify
Copy link

netlify bot commented Oct 30, 2023

Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit eefd552
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/65451f87426ae90008599c73
😎 Deploy Preview https://deploy-preview-2357--gazebo-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #2357 (eefd552) into main (f507526) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2357     +/-   ##
=======================================
- Coverage   96.98   96.97   -0.01     
=======================================
  Files        733     734      +1     
  Lines       8866    8940     +74     
  Branches    2144    2209     +65     
=======================================
+ Hits        8598    8669     +71     
- Misses       265     266      +1     
- Partials       3       5      +2     
Files Coverage Δ
src/pages/OwnerPage/OwnerPage.jsx 100.00% <ø> (ø)
src/services/repos/config.js 100.00% <100.00%> (ø)
src/services/repos/useReposTeam.tsx 95.23% <100.00%> (-0.42%) ⬇️
src/shared/ListRepo/InactiveRepo/InactiveRepo.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ListRepo.jsx 100.00% <100.00%> (ø)
src/shared/ListRepo/NoReposBlock/NoReposBlock.jsx 100.00% <ø> (ø)
...hared/ListRepo/OrgControlTable/OrgControlTable.jsx 100.00% <100.00%> (ø)
...rc/shared/ListRepo/RepoTitleLink/RepoTitleLink.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/NoRepoCoverage.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/ReposTable.jsx 100.00% <ø> (ø)
... and 1 more

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f507526...eefd552. Read the comment docs.

@codecov-staging
Copy link

codecov-staging bot commented Oct 30, 2023

Codecov Report

Merging #2357 (eefd552) into main (f507526) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2357      +/-   ##
==========================================
- Coverage   96.97%   96.96%   -0.01%     
==========================================
  Files         733      734       +1     
  Lines        8866     8940      +74     
  Branches     2190     2222      +32     
==========================================
+ Hits         8598     8669      +71     
- Misses        265      267       +2     
- Partials        3        4       +1     
Files Coverage Δ
src/pages/OwnerPage/OwnerPage.jsx 100.00% <ø> (ø)
src/services/repos/config.js 100.00% <100.00%> (ø)
src/services/repos/useReposTeam.tsx 95.23% <100.00%> (-0.42%) ⬇️
src/shared/ListRepo/InactiveRepo/InactiveRepo.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ListRepo.jsx 100.00% <100.00%> (ø)
src/shared/ListRepo/NoReposBlock/NoReposBlock.jsx 100.00% <ø> (ø)
...hared/ListRepo/OrgControlTable/OrgControlTable.jsx 100.00% <100.00%> (ø)
...rc/shared/ListRepo/RepoTitleLink/RepoTitleLink.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/NoRepoCoverage.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/ReposTable.jsx 100.00% <ø> (ø)
... and 1 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f507526...eefd552. Read the comment docs.

@codecov-public-qa
Copy link

codecov-public-qa bot commented Oct 30, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@f507526). Click here to learn what that means.
The diff coverage is 98.79%.

❗ Current head 7eb59f7 differs from pull request most recent head eefd552. Consider uploading reports for the commit eefd552 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2357   +/-   ##
=======================================
  Coverage        ?   97.16%           
=======================================
  Files           ?      727           
  Lines           ?     8756           
  Branches        ?     2176           
=======================================
  Hits            ?     8508           
  Misses          ?      245           
  Partials        ?        3           
Files Coverage Δ
src/pages/OwnerPage/OwnerPage.jsx 100.00% <ø> (ø)
src/services/repos/config.js 100.00% <100.00%> (ø)
src/services/repos/useReposTeam.tsx 95.23% <100.00%> (ø)
src/shared/ListRepo/InactiveRepo/InactiveRepo.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ListRepo.jsx 100.00% <100.00%> (ø)
src/shared/ListRepo/NoReposBlock/NoReposBlock.jsx 100.00% <ø> (ø)
...hared/ListRepo/OrgControlTable/OrgControlTable.jsx 100.00% <100.00%> (ø)
...rc/shared/ListRepo/RepoTitleLink/RepoTitleLink.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/NoRepoCoverage.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/ReposTable.jsx 100.00% <ø> (ø)
... and 1 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f507526...eefd552. Read the comment docs.

@codecov-qa
Copy link

codecov-qa bot commented Oct 30, 2023

Codecov Report

Merging #2357 (eefd552) into main (f507526) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2357      +/-   ##
==========================================
- Coverage   96.97%   96.96%   -0.01%     
==========================================
  Files         733      734       +1     
  Lines        8866     8940      +74     
  Branches     2195     2178      -17     
==========================================
+ Hits         8598     8669      +71     
- Misses        264      267       +3     
  Partials        4        4              
Files Coverage Δ
src/pages/OwnerPage/OwnerPage.jsx 100.00% <ø> (ø)
src/services/repos/config.js 100.00% <100.00%> (ø)
src/services/repos/useReposTeam.tsx 95.23% <100.00%> (-0.42%) ⬇️
src/shared/ListRepo/InactiveRepo/InactiveRepo.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ListRepo.jsx 100.00% <100.00%> (ø)
src/shared/ListRepo/NoReposBlock/NoReposBlock.jsx 100.00% <ø> (ø)
...hared/ListRepo/OrgControlTable/OrgControlTable.jsx 100.00% <100.00%> (ø)
...rc/shared/ListRepo/RepoTitleLink/RepoTitleLink.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/NoRepoCoverage.jsx 100.00% <ø> (ø)
src/shared/ListRepo/ReposTable/ReposTable.jsx 100.00% <ø> (ø)
... and 1 more

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f507526...eefd552. Read the comment docs.

@RulaKhaled RulaKhaled marked this pull request as ready for review October 31, 2023 12:11
@RulaKhaled RulaKhaled changed the title WIP(?): Team repos list feat: Team plan repos list Oct 31, 2023
drazisil-codecov and others added 18 commits October 31, 2023 14:59
* feat: add header component for team tier customers

* feat: converted Header.jsx to Header.tsx + tests

* fix: add comparison schema types
Filter out events for a given browser because they don't have all JS functions fully implemented causing issues that we cannot address.
…2327)

We will need to hide the flag multi select on the commit detail page when the user is on the team plan as they are not an available feature to those users.

GH codecov/engineering-team#687
…an (#2329)

Disable the flag multi-select on the coverage tab when users/orgs are on a team plan.

GH codecov/engineering-team#685
…2335)

Update indirect changes tab on the commit detail page to grab flags from the url params and pass along as query args.

GH codecov/engineering-team#348
…2334)

Update the files changed tab on the commit detail page to grab flags url param and pass along as query args.

GH codecov/engineering-team#347
* Update with service less requests

* Make sure hook supports service less

* feat: Add Flag MultiSelect to CommitPageTabs (#2303)

Add a feature flagged multi select to the CommitPageTabs component.

GH codecov/engineering-team#344

* Add patch column to pulls table (#2308)

* feat: add patch column to the pulls list page

* uncomment development settings

* feat: Create Team Plan Table for the Files Changed Table on the Commit Detail Page (#2309)

Create new commit fetching hooks for the team plan, as well as creating a new table for files changed tab on the
commit detail page for the new team plan.

GH codecov/engineering-team#633

* Setup pull request page to pass around selected flags (#2282)

* feat: setup pull request page to pass around selected flags in links

* Feedback, fix passing links to files+folders, additional testing

* fix file explorer test failing on href match due to new query param pass through

* airplane commit, cant check local dev server: Resolve merge issues / tests + unify codebases missed of commitSHA and commitSha to just commitSha

* Prevent multislect from collapsing + wire up PR details page to pass through flags links

* Fix accidental removal of ref on usePrefetchPullFileEntry

* Add patch column to pulls table (#2308)

* feat: add patch column to the pulls list page

* uncomment development settings

* feat: Create Team Plan Table for the Files Changed Table on the Commit Detail Page (#2309)

Create new commit fetching hooks for the team plan, as well as creating a new table for files changed tab on the
commit detail page for the new team plan.

GH codecov/engineering-team#633

---------

Co-authored-by: Adrian <[email protected]>
Co-authored-by: nicholas-codecov <[email protected]>

* chore: Remove segment from frontend (#2314)

* Update with tests

* test with support service less

* adjust logic to handle original route

* it's fine it works with no providers

---------

Co-authored-by: nicholas-codecov <[email protected]>
Co-authored-by: Adrian <[email protected]>
Co-authored-by: Terry <[email protected]>
Update PostCSS dependency.
* Migrate textinput to TS

* Add story

* formatting

* organize imports
* feat: Update impacted files resolver for use pull, connect the flag selector to the api.

* update missing logic as requested + unknown flags message to be aligned with repo overview design

* Noticed the changing the pull query broke impacted files while smoke testing, dupicated the same compatibility work for the new resolver.

* Refactor to use a impacted files enum as requested.
Addressing flaky tests in CommitDetailPage and RepoPage.
* restructure folders anticipating second header component for team tier

* feat: add patch coverage section to commit detail page for team tier customer

* fix: rename HeaderDefault to HeaderTeam for team file
* Convert sparkline to typescript

* Consistent type defs

* better variable names
@@ -197,4 +197,26 @@ describe('OrgControlTable', () => {
expect(screen.queryByText(/RepoOrgNotFound/)).not.toBeInTheDocument()
})
})

describe('when show team plan passed in', () => {
it('does nto render the ordering select', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo

const state = sorting.at(0)

if (state) {
const direction = state?.desc
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need state? since you are already checking if state is defined. All good if the IDE is showing an error though.

]
}
return [
columnHelper.accessor('name', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider splitting this into its own variable so you only have to declare it once. Then you can do something like

const nameColumn = columnHelper.accessor('name', {
  header: 'Name',
  id: 'name',
  ...

and then

return [nameColumn, columnHelper.accessor(...), etc].

This will help reduce repeated code in the same file and help with readability.

Copy link
Contributor

@nicholas-codecov nicholas-codecov left a comment

Choose a reason for hiding this comment

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

Few things to take a quick peak at

isCurrentUserPartOfOrg: isCurrentUserPartOfOrg,
}),
getCoreRowModel: getCoreRowModel(),
data: tableData ?? [],
Copy link
Contributor

Choose a reason for hiding this comment

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

?? [] is advised against in the Tanstack Table docs, because if you provide just the empty array it will just create an infinite loop like we were running into before. Can you update the useMemo above to handle the case when table data maybe undefined or null.

Something along the lines:

  const tableData = useMemo(
    () => {
      const data = reposData?.pages?.map((page) => page?.repos).flat()
      return data ?? []
    },
    [reposData?.pages]
  )

Copy link
Contributor

@nicholas-codecov nicholas-codecov left a comment

Choose a reason for hiding this comment

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

LGTM, just one thing to take action on please and thank you

Comment on lines +148 to +152
const repoDisplay = useContext(ActiveContext)
const activated =
repoDisplayOptions[
repoDisplay.toUpperCase() as keyof typeof repoDisplayOptions
]?.status
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a ticket noted down to refactor this context to Typescript and then remove this casting here, just so it's tracked and we can remove it down the road. As well as noting this specific code section here in the ticket.

You can link this doc here for doing context in a typesafe manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RulaKhaled RulaKhaled merged commit fb643b0 into main Nov 3, 2023
27 of 30 checks passed
@RulaKhaled RulaKhaled deleted the team-repos-list branch November 3, 2023 16:36
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.

6 participants