-
Notifications
You must be signed in to change notification settings - Fork 14
Feature/564 open objecten #713
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
base: master
Are you sure you want to change the base?
Conversation
273b407 to
587f7e5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #713 +/- ##
==========================================
+ Coverage 84.98% 85.58% +0.60%
==========================================
Files 141 144 +3
Lines 2843 3018 +175
Branches 224 238 +14
==========================================
+ Hits 2416 2583 +167
- Misses 375 382 +7
- Partials 52 53 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
docker/setup_configuration/data.yaml
Outdated
| '1': | ||
| - record__data__leeftijd | ||
| - record__data__kiemjaar | ||
| # TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the objecttype setup config thing but because of that you cannot really add permissions anymore, should it be added back and if yes only the required objecttype fields or everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for removing it? Is it because you can't create ObjectTypes now without specifying the JSON schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the original use case was to only set the name and service so that all versions with their json schema's will be accesible, then in a tokenauth fields can be specified.
After the objecttype move, we could keep it as just the name, but then specifying the tokenauth permission fields becomes weird because that the schema cannot be defined in in the setup conf.
So i would propose to either:
- remove objecttypes from setup config.
- add the bare minimum (name, name_plural is also required but can be derived) but remove the token auth permission fields.
- add version and schema to objecttype config but i feel like objecttype versions should not be created like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go for the second option for now
| help_text=_("Incremental index number of the object record."), | ||
| ) | ||
| object = models.ForeignKey(Object, on_delete=models.CASCADE, related_name="records") | ||
| version = models.PositiveSmallIntegerField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version could become an FK?
stevenbal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall 👍
docker/setup_configuration/data.yaml
Outdated
| '1': | ||
| - record__data__leeftijd | ||
| - record__data__kiemjaar | ||
| # TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for removing it? Is it because you can't create ObjectTypes now without specifying the JSON schema?
|
@Floris272 will you do the docs in a separate PR? |
b4b24a2 to
d81a31c
Compare
| "service", | ||
| ], # TODO remove service from unique_fields after objecttype migration since it will no longer be part of the ObjectType model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command could be kept, Objecttypes will just be unique on their uuid only
yes |
5a4d32e to
d673439
Compare
stevenbal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small things remaining still, I'd recommend to split off the duplicate UUID stuff into a separate PR, because this PR has been open for quite a while already
src/objects/core/management/commands/check_for_external_objecttypes.py
Outdated
Show resolved
Hide resolved
docker/setup_configuration/data.yaml
Outdated
| '1': | ||
| - record__data__leeftijd | ||
| - record__data__kiemjaar | ||
| # TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go for the second option for now
d673439 to
8214fae
Compare
…VERSION_CACHE_TIMEOUT
5e51c56 to
d9f47d4
Compare
|
| Branch | feature/564-open-objecten |
| Testbed | ubuntu-24.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| performance_test/tests/test_objects_list.py::test_objects_api_list_filter_by_object_type | 📈 view plot 🚷 view threshold | 121.91 ms(-0.94%)Baseline: 123.07 ms | 129.22 ms (94.34%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_filter_one_result | 📈 view plot 🚷 view threshold | 18.55 ms(-9.86%)Baseline: 20.58 ms | 21.61 ms (85.85%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_large_page_size_page_1 | 📈 view plot 🚷 view threshold | 327.06 ms(+4.07%)Baseline: 314.28 ms | 329.99 ms (99.11%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_large_page_size_page_5 | 📈 view plot 🚷 view threshold | 326.21 ms(+4.45%)Baseline: 312.31 ms | 327.93 ms (99.48%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_small_page_size_page_20 | 📈 view plot 🚷 view threshold | 122.65 ms(-2.78%)Baseline: 126.16 ms | 132.47 ms (92.59%) |
Fixes #564
Changes
TODO