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

Data Table: Bot Accounts #849

Merged
merged 22 commits into from
Dec 19, 2024
Merged

Data Table: Bot Accounts #849

merged 22 commits into from
Dec 19, 2024

Conversation

ChrisHuynh333
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 commented Nov 22, 2024

What does this PR do and why?

Note: Due to the size of this current PR, the following will be branched out and fixed in separate PRs:

  • Removal of new PAT panel upon bot deletion and PAT revoke

This PR refactors the bot account pages (Group and Project) to use the data_table_component (includes PAT tables to use data_table_component). This required much of the response logic (turbo etc.) to be re-done.

Screenshots or screen recordings

image

image

How to set up and validate locally

Test all bot functionality:

  1. Create new bot (table updates with new bot and new PAT panel appears/updates)
  2. Delete bot (table updates, empty state appears if there are 0 bots)
  3. Active PATs for a bot and Revoke action (empty state of PAT table once all PATs are revoked)
  4. Generate new PAT (table active PAT # updates, new PAT panel appears/updates)

PR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

@ChrisHuynh333 ChrisHuynh333 force-pushed the data-table-bot-accounts branch 4 times, most recently from dd41321 to c80feb1 Compare November 28, 2024 22:15

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@ChrisHuynh333 ChrisHuynh333 self-assigned this Dec 2, 2024
@ChrisHuynh333 ChrisHuynh333 added the ready for review Pull request is ready for review label Dec 2, 2024
@ChrisHuynh333 ChrisHuynh333 marked this pull request as ready for review December 2, 2024 16:42

This comment has been minimized.

@ChrisHuynh333 ChrisHuynh333 force-pushed the data-table-bot-accounts branch from 09fcd55 to 4cf2c7a Compare December 2, 2024 19:02

This comment has been minimized.

Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

This looks really great overall.  I actually only have one wording change I would like to see.

config/locales/en.yml Outdated Show resolved Hide resolved
@ChrisHuynh333 ChrisHuynh333 force-pushed the data-table-bot-accounts branch from 4cf2c7a to 448c1ca Compare December 4, 2024 14:59

This comment has been minimized.

This comment has been minimized.

deepsidhu85
deepsidhu85 previously approved these changes Dec 9, 2024
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@joshsadam
Copy link
Contributor

I found a minor UI bug that I am hoping you can fix, and I am hoping that I can explain it properly. I will paste the screenshot and try to explain below.
bots

  • The div highlighted in the developer console wraps a turbo-frame.
  • When the turbo frame is empty (as in the screenshot) the div is empty but still renders and has the class of mb-4 which for all purposes makes the margin bottom on the header 8.
  • Can you remove the wrapping div and set the mb-4 on the content within the turbo-frame so that it is only there if the turbo-frame has content?

Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

This looks good. I left 2 comments that I am hoping you can quickly fix. Good work on this.

@ChrisHuynh333
Copy link
Collaborator Author

I found a minor UI bug that I am hoping you can fix, and I am hoping that I can explain it properly. I will paste the screenshot and try to explain below. bots

  • The div highlighted in the developer console wraps a turbo-frame.
  • When the turbo frame is empty (as in the screenshot) the div is empty but still renders and has the class of mb-4 which for all purposes makes the margin bottom on the header 8.
  • Can you remove the wrapping div and set the mb-4 on the content within the turbo-frame so that it is only there if the turbo-frame has content?

Thanks, removed the wrapper and moved the mb-4 to the frame content for both projects and groups in f039c40

This comment has been minimized.

Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

This looks good to me

Copy link

Code Metrics Report

Coverage Test Execution Time
92.7% 9m49s

Code coverage of files in pull request scope (99.1%)

Files Coverage
app/components/bots/table_component.html.erb 96.6%
app/components/bots/table_component.rb 100.0%
app/components/personal_access_tokens/table_component.html.erb 95.6%
app/components/personal_access_tokens/table_component.rb 100.0%
app/controllers/concerns/bot_actions.rb 97.4%
app/controllers/concerns/bot_personal_access_token_actions.rb 97.5%
app/controllers/groups/bots_controller.rb 100.0%
app/controllers/projects/bots_controller.rb 100.0%
app/views/groups/bots/_access_token_section.html.erb 100.0%
app/views/groups/bots/_bot_modal.html.erb 100.0%
app/views/groups/bots/_table.html.erb 100.0%
app/views/groups/bots/create.turbo_stream.erb 100.0%
app/views/groups/bots/index.html.erb 100.0%
app/views/groups/bots/personal_access_tokens/_generate_personal_access_token_modal.html.erb 100.0%
app/views/groups/bots/personal_access_tokens/_personal_access_token_listing_modal.html.erb 100.0%
app/views/groups/bots/personal_access_tokens/_table.html.erb 100.0%
app/views/groups/bots/personal_access_tokens/create.turbo_stream.erb 100.0%
app/views/groups/bots/personal_access_tokens/revoke.turbo_stream.erb 100.0%
app/views/profiles/personal_access_tokens/_table.html.erb 100.0%
app/views/projects/bots/_access_token_section.html.erb 100.0%
app/views/projects/bots/_bot_modal.html.erb 100.0%
app/views/projects/bots/_table.html.erb 100.0%
app/views/projects/bots/create.turbo_stream.erb 100.0%
app/views/projects/bots/index.html.erb 100.0%
app/views/projects/bots/personal_access_tokens/_generate_personal_access_token_modal.html.erb 100.0%
app/views/projects/bots/personal_access_tokens/_personal_access_token_listing_modal.html.erb 100.0%
app/views/projects/bots/personal_access_tokens/_table.html.erb 100.0%
app/views/projects/bots/personal_access_tokens/create.turbo_stream.erb 100.0%
app/views/projects/bots/personal_access_tokens/index.turbo_stream.erb 100.0%
app/views/projects/bots/personal_access_tokens/revoke.turbo_stream.erb 100.0%
app/views/workflow_executions/submissions/create.turbo_stream.erb 100.0%

Reported by octocov

@joshsadam joshsadam merged commit 13ecb2f into main Dec 19, 2024
4 checks passed
@joshsadam joshsadam deleted the data-table-bot-accounts branch December 19, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants