HJ-97 - Update systems endpoint to filter vendor deleted systems #5553
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides
|
||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
refs/pull/5553/merge
|
| Run status |
|
| Run duration | 00m 49s |
| Commit |
|
| Committer | Andres Torres |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
OK, this is looking good in some brief manual testing - I'll tentatively approve, but a few notes:
- i left a comment on the ticket to confirm with product that we're OK filtering these out of the system list endpoint (and system inventory page) while not filtering out of the reporting pages
- there are some minor points in the code about backward compatibility/stylistic points that'd be nice to address
- it's probably worth improving the "steps to confirm" in the PR description, since adding a "vendor deleted date" is not a self-explanatory step, since the UI doesn't support manually editing this field (i don't think?). i'd either:
- provide steps on how to test this integrated with fidesplus - which would consist of creating a system based on a compass vendor who actually has a
vendor_deleted_datepopulated. this is the best test, since it more closely mimics the actual use case in the ticket. note that i didn't complete this testing myself. - provide a sample API request that you used to set a
vendor_deleted_date"manually" on a system. this is probably good enough to get the PR merged, but i do think that generally we should make sure there are some testing steps listed out here for the "end to end" functionality, which includes vendors from compass withvendor_deleted_dates.
- provide steps on how to test this integrated with fidesplus - which would consist of creating a system based on a compass vendor who actually has a
| result = await db.execute(duplicates_removed) | ||
| return result.scalars().all() |
There was a problem hiding this comment.
any particular reason that we're changing the functionality here? if it's just a general optimization, i understand that motivation - but i think i'd lean toward keeping this using the simple return await list_resource(System, db), because i believe this was in place for backward compatibility of this endpoint with uses in the fides CLI, etc - when it specifically did not paginate, filter, or deduplicate (or do anything besides list all the systems, raw).
granted, that backward compatibility is not well commented in the code, so if we do keep this as it was, then a nice addition would be a code comment to clarify why it's kept as list_resource, i.e. for backward compatibility reasons.
i see/know that @erosselli and @galvana worked on refactoring this endpoint a little while back, may be worth getting them to chime in quickly.
also, as more of a stylistic comment - i preferred this conditional coming at the beginning of the endpoint function, as it had been previously. if there's a branch of the function that's a "special case" and returns immediately, i'd prefer to see that at the beginning - i just find that easier to make sense of when reading the function...
There was a problem hiding this comment.
Hey guys, yes, that's the reason we kept the non-paginated version of the system retrieval, for backwards compatibility. I also agree with @adamsachs's comment about having this higher up so it's a little more obvious that there's a possible early return.
| "vendor_deleted_date, expected_systems_count, show_deleted", | ||
| [ | ||
| (datetime.now() - timedelta(days=1), 1, True), | ||
| (datetime.now() - timedelta(days=1), 0, None), |
There was a problem hiding this comment.
nit - reads a bit weird to me that the value here is True or None, although i know that None is falsey 😛
| (datetime.now() - timedelta(days=1), 0, None), | |
| (datetime.now() - timedelta(days=1), 0, False), |
There was a problem hiding this comment.
More than falsey, I was trying to test the parameter being not present.
fides
|
||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 58s |
| Commit |
|
| Committer | Andres Torres |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
4
|
| View all changes introduced in this branch ↗︎ | |
Closes #HJ-97
Description Of Changes
Update systems endpoint to filter vendor deleted systems
Code Changes
/systemlist endpoint to filter out vendor deleted systems.Steps to Confirm
For System resource:
vendor_deleted_atfield for the newly created system to something in the past (if it is null or in the future it won't be filtered out)/systemthe created system should be filtered out.Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works