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

Merge RTK APIs #3059

Merged
merged 12 commits into from
May 3, 2023
Merged

Merge RTK APIs #3059

merged 12 commits into from
May 3, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Apr 12, 2023

Closes #1386

Code Changes

  • Merges all of our createApis into one!
    • ...almost. the exception is the healthApi since that one isn't prefixed by /api/v1. we could write a wrapper around our baseApi if we really wanted them all to use the same one, but the healthApi is a bit of an outlier for not being under /api/v1 imo, so I left it as-is for now
  • Fix some backend endpoints to use the APIRouter that handles trailing slashes
    • This became apparent because many of the traditional "ops" query calls were not using nextjs rewrites, and instead were swapping out the query call to localhost:8080. this is not ideal, since then you can run into cors problems when developing locally (I think our backend might allow cors to work around this though, I haven't looked into this recently). Once I updated those endpoints to use nextjs rewrites, the trailing slash problem cropped up since next always adds a trailing slash
  • Consolidate data category API calls. The datamap UI had a separate API for this, now it uses the same one
  • Refactor store.ts to be more consistent and make more sense (imo 😄 )
  • Use consistent tag names (we had i.e. "CustomFieldDefinition" and "Data Category". I made them all space separated since RTK can handle it and I think it's more readable that way)

Steps to Confirm

  • Everything still works 🤞 Would be good to run the QA tests here since this touches nearly all of the API calls the FE makes

Pre-Merge Checklist

Description Of Changes

Write some things here about the changes and any potential caveats

@cypress
Copy link

cypress bot commented Apr 12, 2023

Passing run #1727 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 79b0fb6 into 1aa3219...
Project: fides Commit: 033e1aaaac ℹ️
Status: Passed Duration: 00:45 💡
Started: May 3, 2023 12:50 PM Ended: May 3, 2023 12:50 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (1aa3219) 87.37% compared to head (267188b) 87.39%.

❗ Current head 267188b differs from pull request most recent head 79b0fb6. Consider uploading reports for the commit 79b0fb6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3059      +/-   ##
==========================================
+ Coverage   87.37%   87.39%   +0.02%     
==========================================
  Files         313      313              
  Lines       18241    18242       +1     
  Branches     2366     2365       -1     
==========================================
+ Hits        15938    15943       +5     
+ Misses       1871     1867       -4     
  Partials      432      432              
Impacted Files Coverage Δ
.../ops/api/v1/endpoints/connection_type_endpoints.py 100.00% <100.00%> (ø)
src/fides/lib/oauth/api/routes/user_endpoints.py 95.45% <100.00%> (+0.05%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@allisonking allisonking force-pushed the aking/1386/merge-rtk-apis branch 2 times, most recently from 00d7062 to 0a4bb46 Compare April 27, 2023 16:52
@allisonking allisonking changed the title Merge RTK slices Merge RTK APIs Apr 27, 2023
@allisonking allisonking marked this pull request as ready for review April 27, 2023 18:53
@TheAndrewJackson
Copy link
Contributor

@allisonking This is awesome! It's great to have everything be unified together.

I'm running into some issues with logging in. I'm using nox -s "dev(slim)" in plus. When I try to login I get a 405.

Did you experience this at all while developing?

@allisonking
Copy link
Contributor Author

@allisonking This is awesome! It's great to have everything be unified together.

I'm running into some issues with logging in. I'm using nox -s "dev(slim)" in plus. When I try to login I get a 405.

Did you experience this at all while developing?

Ah yeah auth was one of the endpoints that needed the trailing slash fix, which is only in this branch. So when you do a nox -s "dev(slim)" in plus, it doesn't get that fix yet (at least that's my theory!)

See if things work for you using nox -s dev in fides first, then maybe we can use some of Adam's fancy new release branch tagging to test things out with plus! (I think we'd have to create new tags, and I don't know if that would interfere with the ongoing release right now)

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

@allisonking The changes not being propagated to fidesplus was the issue. Everything is working great!

@allisonking allisonking merged commit 8d9ed30 into main May 3, 2023
@allisonking allisonking deleted the aking/1386/merge-rtk-apis branch May 3, 2023 13:07
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.

Combine our RTK API's into one API instance for better cache re-fetching and tags
2 participants