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

support active property on taxonomy elements #3784

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Jul 14, 2023

Closes #3709

Description Of Changes

Extends taxonomy pydantic models defined in fideslang to add an active boolean property, and updates the corresponding db model for these taxonomy elements. This enables us to update and persist this property on taxonomy elements via the CRUD API.

Code Changes

  • extend fideslang pydantic models for taxonomy elements to support an active boolean field
  • adds a corresponding an active property to the db model for taxonomy elements: DataUse, DataSubject, DataQualifier, DataCategory
    • migration establishes a server_default value of True for the property, which means that all existing taxonomy elements on the system automatically get a value of True
  • add some test coverage around the API specifically for this new property, ensuring we can create, update and read it

Steps to Confirm

  • a manual test of some of the CRUD APIs for taxonomy elements that ensures we can create/update/read the active property should probably be sufficient here! see screenshot below for one of those tests
Screenshot 2023-07-14 at 11 11 01 AM

Pre-Merge Checklist

@cypress
Copy link

cypress bot commented Jul 14, 2023

Passing run #3202 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge a9c51de into aa76b4b...
Project: fides Commit: c39d1b365e ℹ️
Status: Passed Duration: 00:58 💡
Started: Jul 19, 2023 11:27 AM Ended: Jul 19, 2023 11:28 AM

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

@adamsachs
Copy link
Contributor Author

hey @ThomasLaPiana - i might be wrong, but i think the mypy failures i'm seeing on this branch right now are due to the packaging updates made to fideslang in 1.4.3. the fideslang commit that this branch is pinned to is ahead of 1.4.3, and i don't think we've tried integrating fideslang==1.4.3 into fides yet. i get the same mypy errors locally if i pin fides to fideslang==1.4.3.

i did a little bit of digging on the packaging updates and nothing stood out to me immediately - though i did notice this discrepancy in the unzipped wheels of the two package versions. perhaps it's something to do with the [tool.setuptools.packages.find] section? i was going to keep on digging but i'm getting to the end of my day here and it's a bit too much of a head-scratcher for this late on a friday 😅

(you can ignore the other CI failures on this branch for now, it seems like some unrelated blip retrieving a JS resource)

@ThomasLaPiana
Copy link
Contributor

hey @ThomasLaPiana - i might be wrong, but i think the mypy failures i'm seeing on this branch right now are due to the packaging updates made to fideslang in 1.4.3. the fideslang commit that this branch is pinned to is ahead of 1.4.3, and i don't think we've tried integrating fideslang==1.4.3 into fides yet. i get the same mypy errors locally if i pin fides to fideslang==1.4.3.

i did a little bit of digging on the packaging updates and nothing stood out to me immediately - though i did notice this discrepancy in the unzipped wheels of the two package versions. perhaps it's something to do with the [tool.setuptools.packages.find] section? i was going to keep on digging but i'm getting to the end of my day here and it's a bit too much of a head-scratcher for this late on a friday 😅

(you can ignore the other CI failures on this branch for now, it seems like some unrelated blip retrieving a JS resource)

Ah, I see...not sure why the packaging changes would affect anything other than I fixed some things, which with mypy often lead to more breaking hah.

We could try to just ignore fideslang stuff altogether...we do that with lots of other packages 🤔

@ThomasLaPiana
Copy link
Contributor

I'm playing around with the mypy stuff...I'll let you know if I get anywhere!

@adamsachs adamsachs marked this pull request as ready for review July 19, 2023 11:54
@ThomasLaPiana
Copy link
Contributor

@adamsachs now that we've backed out the fideslang change we have a stay of execution on those mypy issues....

@adamsachs
Copy link
Contributor Author

adamsachs commented Jul 19, 2023

@ThomasLaPiana another one for you when you've got a chance. This one should (hopefully) be relatively quick - i think where i landed is basically what you'd suggested on the fideslang issue!

@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented Jul 19, 2023

@adamsachs looks like ops integration tests are failing?

Edit: oh right, because of pymssql

@adamsachs
Copy link
Contributor Author

@adamsachs now that we've backed out the fideslang change we have a stay of execution on those mypy issues....

😂 welllll not quite because the issues are still on 1.4.3 and we needed to pull that in (already in main!) for the pyyaml fix.

that being said, IMO the mypy errors aren't showstoppers - but we do need to figure out what to do about mssql before releasing, i think!

@ThomasLaPiana
Copy link
Contributor

@adamsachs now that we've backed out the fideslang change we have a stay of execution on those mypy issues....

😂 welllll not quite because the issues are still on 1.4.3 and we needed to pull that in (already in main!) for the pyyaml fix.

that being said, IMO the mypy errors aren't showstoppers - but we do need to figure out what to do about mssql before releasing, i think!

oh right.....well no worries! we can just ship it with the mypy breaks and then I'll work on the fix. I think the easiest fix would be rewriting the entire thing in Rust so mypy stops annoying me 😂

Copy link
Contributor

@ThomasLaPiana ThomasLaPiana left a comment

Choose a reason for hiding this comment

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

Looks great! Excellent solution/execution

I played around with the default taxonomy and confirmed they come back as true

image

@adamsachs
Copy link
Contributor Author

Looks great! Excellent solution/execution

I played around with the default taxonomy and confirmed they come back as true

image

thanks a bunch for the quick review @ThomasLaPiana and the extra confirmation!!! 👍

@adamsachs adamsachs merged commit 361eb72 into main Jul 19, 2023
@adamsachs adamsachs deleted the asachs/fides-3709 branch July 19, 2023 16:27
@adamsachs
Copy link
Contributor Author

@jpople just an FYI that now that this is merged into main, it should unblock you for #3710.

the update here to the API is that the endpoints used for CRUD operations on taxonomy elements now take an additional active boolean field. the default on this field is True, if not specified on a create or update operation. If you pull main, the generated swagger API docs localhost:8080/docs should be updated to include this API update and allow you to play around with it - the screenshot in the PR description can also be a quick pointer.

but please don't hesitate to reach out if any of that isn't clear or if it doesn't get you what you need to make the FE updates!

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.

BE: make it possible to disable taxonomy elements and have them not show in picker
2 participants