LITE-27896 Add PPR endpoints #51
Conversation
| summary=summary, | ||
| status=status, | ||
| created_by=context.user_id, | ||
| **config_kwargs, | ||
| ) | ||
| db.set_verbose(new_ppr) | ||
| db.commit() | ||
| return new_ppr | ||
| return new_ppr, file_instance, active_configuration |
There was a problem hiding this comment.
I know that it looks bad to return 3 values in a function.
But we are going to refactor this function anyway, so let's refactor this place with a function.
There was a problem hiding this comment.
I think that this could be solved by adding relationship on PPRVersion to Configurations and Files, like:
configuration = relationship('Configuration', foreign_keys="Configuration.id")
file = relationship('File', foreign_keys="File.id")
So then you can access new properties like: new_ppr.configuration or new_ppr.file
There was a problem hiding this comment.
I'm not sure about the file relation here... We already have a file link here, but I think that we should store a reference to the configuration that was used to generate the PPR. I'm ok to merge this now, but @Hairash submit a separate issue and patch to add the reference to the latest configuration in case of:
- File is generated on the new product version appears
- File is generated when user click on the regenerate button
| description_items.append(ppr.description) | ||
| if ppr.summary: | ||
| description_items.append(str(ppr.summary)) | ||
| description = '\n'.join(description_items) |
There was a problem hiding this comment.
Better to decide, how to output description and summary. Now I merge them because there are case when two of them are present and make sense. Like upload file with a description, but facing the validation errors.
There was a problem hiding this comment.
I have new task to implement this, but for now is good to have at least this approach
| @@ -298,4 +306,6 @@ def test_create_ppr_db_error( | |||
| ppr_data, common_context, deployment, | |||
| dbsession, connect_client, logger, | |||
| ) | |||
| assert ex.value.message == "Database error occurred." | |||
| assert ex.value.message == ( | |||
There was a problem hiding this comment.
I think better to come up with another test for DB error, because now it is duplicated with another one that file already exists.
| @@ -117,6 +118,8 @@ def create_ppr(ppr, context, deployment, db, client, logger): | |||
| new_version = get_ppr_new_version(db, deployment) | |||
| config_kwargs = {} | |||
| status = PPRVersion.STATUS.ready | |||
| active_configuration = None | |||
| product_version = None | |||
There was a problem hiding this comment.
[COD-B] Why we have the product_version as None when user upload a file?, I think the new ppr has to correspond to the product version available at that moment on time no matter where the new file been generated from.
There was a problem hiding this comment.
As I understand, no.
@d3rky tell us.
There was a problem hiding this comment.
We cannot guarantee with what version of the product and configuration this PPR was created. That's why it is null. We can assume, that probably it is the latest versions... But who knows? Maybe the version was not released at that moment
There was a problem hiding this comment.
Sorry was my misunderstanding, then lets proceed!
| description_items.append(ppr.description) | ||
| if ppr.summary: | ||
| description_items.append(str(ppr.summary)) | ||
| description = '\n'.join(description_items) |
There was a problem hiding this comment.
I have new task to implement this, but for now is good to have at least this approach
| summary=summary, | ||
| status=status, | ||
| created_by=context.user_id, | ||
| **config_kwargs, | ||
| ) | ||
| db.set_verbose(new_ppr) | ||
| db.commit() | ||
| return new_ppr | ||
| return new_ppr, file_instance, active_configuration |
There was a problem hiding this comment.
I think that this could be solved by adding relationship on PPRVersion to Configurations and Files, like:
configuration = relationship('Configuration', foreign_keys="Configuration.id")
file = relationship('File', foreign_keys="File.id")
So then you can access new properties like: new_ppr.configuration or new_ppr.file
| @@ -316,6 +317,53 @@ def get_configuration_schema(configuration, file): | |||
| ) | |||
|
|
|||
|
|
|||
| def get_ppr_version_schema(ppr, file, conf): | |||
There was a problem hiding this comment.
Same in here, if you relay on relationships, it only gonna take ppr arg
| @@ -117,6 +118,8 @@ def create_ppr(ppr, context, deployment, db, client, logger): | |||
| new_version = get_ppr_new_version(db, deployment) | |||
| config_kwargs = {} | |||
| status = PPRVersion.STATUS.ready | |||
| active_configuration = None | |||
| product_version = None | |||
There was a problem hiding this comment.
We cannot guarantee with what version of the product and configuration this PPR was created. That's why it is null. We can assume, that probably it is the latest versions... But who knows? Maybe the version was not released at that moment
| summary=summary, | ||
| status=status, | ||
| created_by=context.user_id, | ||
| **config_kwargs, | ||
| ) | ||
| db.set_verbose(new_ppr) | ||
| db.commit() | ||
| return new_ppr | ||
| return new_ppr, file_instance, active_configuration |
There was a problem hiding this comment.
I'm not sure about the file relation here... We already have a file link here, but I think that we should store a reference to the configuration that was used to generate the PPR. I'm ok to merge this now, but @Hairash submit a separate issue and patch to add the reference to the latest configuration in case of:
- File is generated on the new product version appears
- File is generated when user click on the regenerate button
Hairash
force-pushed
the
LITE-27896-ppr-endpoints
branch
from
b7b3ff1 to
c509f6e
Compare
No description provided.