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

feat: added description and comment (#284) #344

Closed

Conversation

ShubhranshuSanjeev
Copy link
Collaborator

Problem

Describe the problem you are trying to solve here

Solution

Provide a brief summary of your solution so that reviewers can understand your code

Environment variable changes

What ENVs need to be added or changed

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

Things needed to be done after deploying this change (if any)

API changes

Endpoint Method Request body Response Body
API GET/POST, etc request response

Possible Issues in the future

Describe any possible issues that could occur because of this change

Copy link
Collaborator

@Datron Datron left a comment

Choose a reason for hiding this comment

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

The additions to ramp and conclude form UI to take change reason will come in another PR?

let last_modified_time = Utc::now().naive_utc();
let config_version_id =
conn.transaction::<_, superposition::AppError, _>(|transaction_conn| {
for (context_weight, context_id) in contexts_new_weight {
for (context_weight, context_id, _description, _change_reason) in contexts_new_weight.clone() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why _description and _change_reason? Are we not using these variables anywhere?

@@ -87,6 +112,7 @@ async fn update_type(
type_templates::type_schema.eq(request.clone()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey so there is an issue here. Request now contains description and change reason, which cannot directly be inserted into the column type_schema. We need to extract type schema. It would be better to create a type TypeTemplateUpdateRequest. This can be taken from org__id-test branch, or when we rebase we can keep that code

@Datron Datron self-requested a review January 17, 2025 06:53
@Datron Datron marked this pull request as ready for review January 17, 2025 06:53
@Datron Datron requested a review from a team as a code owner January 17, 2025 06:53
@ShubhranshuSanjeev
Copy link
Collaborator Author

Closing this PR, the commit is there in the org__id-test branch.

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.

3 participants