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: Add role api keys in UI #3042

Merged
merged 28 commits into from
Feb 16, 2024
Merged

feat: Add role api keys in UI #3042

merged 28 commits into from
Feb 16, 2024

Conversation

novakzaballa
Copy link
Contributor

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

  • Add the ability in the frontend to create role API keys

How did you test this code?

  • Go to manage -> Keys
  • Click the "Create API Key" button
  • Disable the "Is admin" switch and add roles

Copy link

vercel bot commented Nov 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 15, 2024 2:09pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 15, 2024 2:09pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 15, 2024 2:09pm

@github-actions github-actions bot added front-end Issue related to the React Front End Dashboard api Issue related to the REST API labels Nov 27, 2023
Copy link
Contributor

github-actions bot commented Nov 27, 2023

Uffizzi Preview deployment-44580 was deleted.

@novakzaballa novakzaballa changed the title feat: Add role api key in UI feat: Add api key role in UI Nov 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (c60a145) 95.93% compared to head (bd51477) 95.92%.
Report is 8 commits behind head on main.

Files Patch % Lines
api/api_keys/models.py 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3042      +/-   ##
==========================================
- Coverage   95.93%   95.92%   -0.01%     
==========================================
  Files        1088     1088              
  Lines       33818    33825       +7     
==========================================
+ Hits        32443    32447       +4     
- Misses       1375     1378       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@novakzaballa novakzaballa changed the title feat: Add api key role in UI feat: Add role api keys in UI Nov 27, 2023
@novakzaballa novakzaballa requested review from a team and matthewelwell and removed request for a team November 27, 2023 19:24
api/api_keys/models.py Outdated Show resolved Hide resolved
api/api_keys/serializers.py Outdated Show resolved Hide resolved
api/api_keys/serializers.py Outdated Show resolved Hide resolved
Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

Code looks ok, but as per previous comment, it doesn't appear to be working. Mostly leaving this review comment to remove this PR from my list because GH doesn't seem to have a function to do it another way.

@novakzaballa
Copy link
Contributor Author

novakzaballa commented Jan 29, 2024

@novakzaballa I've just tested this but it seems as though the roles aren't getting applied to the keys correctly.

@matthewelwell I found the issue, and now is solved, I had to split the backend: #3346

@matthewelwell
Copy link
Contributor

matthewelwell commented Feb 15, 2024

Yep, seems like it works now which is great! A couple of minor things:

  1. This shouldn't say 'Terraform API Keys' any more.
Screenshot 2024-02-15 at 11 45 16
  1. Can we add an item to the table indicating whether a key is an admin or not?
  2. We need to add / update the docs. Currently they're linking to the Terraform documentation which is not correct. Perhaps we should create a separate PR for the docs?

@novakzaballa
Copy link
Contributor Author

Yep, seems like it works now which is great! A couple of minor things:

  1. This shouldn't say 'Terraform API Keys' any more.

  2. Can we add an item to the table indicating whether a key is an admin or not?

  3. We need to add / update the docs. Currently they're linking to the Terraform documentation which is not correct. Perhaps we should create a separate PR for the docs?

Hey @matthewelwell, I changed the title and added a field in the API keys table to see if it is an admin API key or not.

I will create an issue for the API keys documentation.

Copy link
Contributor

@matthewelwell matthewelwell 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 now, but one improvement that I'd like to see (which can maybe be done in a separate PR) is some kind of indication that a key is active / expired. I've added a has_expired parameter to the API here I think we should just use that and display it in the table for now?

@novakzaballa novakzaballa added this pull request to the merge queue Feb 16, 2024
Merged via the queue into main with commit b746d09 Feb 16, 2024
23 checks passed
@novakzaballa novakzaballa deleted the feat/add-role-api-key-in-ui branch February 16, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API front-end Issue related to the React Front End Dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants