-
Notifications
You must be signed in to change notification settings - Fork 27
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
✨ Is765/extends products plugin (⚠️ devops) #3540
✨ Is765/extends products plugin (⚠️ devops) #3540
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3540 +/- ##
========================================
- Coverage 83.5% 82.1% -1.4%
========================================
Files 852 802 -50
Lines 35978 34193 -1785
Branches 759 616 -143
========================================
- Hits 30054 28105 -1949
- Misses 5727 5921 +194
+ Partials 197 167 -30
Flags with carried forward coverage won't be shown. Click here to find out more.
|
02d2348
to
2a0e22d
Compare
ab93e75
to
3194bad
Compare
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.
great, just one thing is that the migration should actually take care of migrating the data. else that means you require manual changes (as stated in the description, but that goes against having a migration that works out of the box).
|
||
|
||
def upgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### |
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.
Q: why is there no migration script for the data there? Same for the downgrade.
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.
In the interest of time, @odeimaiz and me decided that this had a very low return of investment (would have to pass str columns to json columns).
I think in this case it is "affordable" since this is read-only data and inserted manually via adminer. In addition, it is version-controlled separately in the repo indicated above.
what do you think?
packages/postgres-database/src/simcore_postgres_database/models/products.py
Show resolved
Hide resolved
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.
The exposed frontend-data.json is exactly what we need. Thanks!
services/web/server/src/simcore_service_webserver/products_model.py
Outdated
Show resolved
Hide resolved
31e7f40
to
8e1a0e7
Compare
8e1a0e7
to
8d8847a
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
What do these changes do?
This PR extends the functionality of the product's plugin to support new requirements in the front-end (see PR #3534)
product
's table (see (postgres_database/models/products.py
GET /static-frontend-data.json
response includes ONLY information of selected productproducts
tables, i.e. all entries inproducts
table must be reconfigured after migration !!!osparc
product in https://git.speag.com/oSparc/productsfront-end
New
GET /static/frontend-data.json
@odeimaiz I think the viewer needs some changes so it can render objects (i.e. trees). Otherwise, just show the json as is (at the end of the day, this is only for testers)
Related issue/s
How to test
Checklist
make version-*
make openapi.json
cd packages/postgres-database
,make setup-commit
,sc-pg review -m "my changes"