-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: fix github#1988 #1995
fix: fix github#1988 #1995
Conversation
--bug=1051321 --user=王孝刚 【github#1988】【应用】应用编排接入带用户输入参数的应用,选择后,再次进入应用,选择的参数消失了 https://www.tapd.cn/57709429/s/1643562 --bug=1051323 --user=王孝刚 【应用】创建应用报错 https://www.tapd.cn/57709429/s/1643571
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
dataset_list))] | ||
self.update_search_node(embed_application.work_flow, [str(dataset.get('id')) for dataset in dataset_list]) | ||
return {**ApplicationSerializer.Query.reset_application(ApplicationSerializerModel(embed_application).data), | ||
'dataset_id_list': dataset_id_list} | ||
|
||
class ApplicationKeySerializerModel(serializers.ModelSerializer): | ||
class Meta: |
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 code provided appears to have several potential issues:
-
Imports and Class Definitions: The imports at the top appear correct, but there might be unused import statements which could cause confusion.
-
get_application
Method:- If
with_valid
is set toTrue
, it checks if the current user (self
) is valid. However, there is no method namedis_valid()
defined within theProfileAPI
class. - For non-embedded applications (i.e., when
with_valid
is false), the code retrieves an application using its ID and then returns an updated version of it using a serializer. This is done correctly.
- If
-
Custom Theme Handling: There is unnecessary complexity with default font colors handled directly rather than using a function or template context.
-
Work Flow Version Selection: The selection of the latest Work Flow version in embedded applications seems redundant since the method would only return the most recent version regardless of whether embedded or standalone.
-
Database Queries: While the logic looks efficient, some queries can be optimized, particularly ensuring that related data like datasets is fetched concurrently where possible and avoiding nested calls like
list_dataset
.
Here is a brief refactored snippet with improvements mentioned above:
def update_search_node(work_flow_id, dataset_ids):
# Assuming update_search_node handles this operation efficiently
pass
class ProfileAPI(APIViewBase):
@staticmethod
def get_embed(with_valid=True, params=None):
# Ensure proper error handling
if params is None:
params = {}
# Get other necessary data here...
@staticmethod
def get_application(app_id, with_valid=True):
if with_valid:
raise AppUnauthorizedFailed(500, "非法用户") # Correct exception raising
try:
embed_application = QuerySet(Application).get(id=app_id)
if app_instance.type == ApplicationTypeChoices.WORK_FLOW:
versions_query_set = query_work_flow_versions(embed_application.id).latest()
embed_application.work_flow = versions_query_set.workflow
dataset_list = self.list_dataset(with_valid=False)
mapping_dataset_id_list = QuerySet(ApplicationDatasetMapping).filter(application_id=app_id)
dataset_id_list = [d['id'] for d in filter(lambda x: x in map(str, mapping_dataset_id_list), dataset_list)]
embed_app_data = ApplicationSerializer.Operate(data={'user_id': embed_application.user_id, 'application_id': app_id})
updated_app_data = {**embed_app_data.one(), 'dataset_id_list': dataset_id_list}
return updated_app_data
except DoesNotExist:
raise NotFound404()
# Helper functions omitted for brevity
This should improve readability and reduce redundancy while maintaining core functionality.
} else { | ||
return item | ||
} | ||
}) | ||
console.log(merge_user_input_field_list) | ||
set( | ||
props.nodeModel.properties.node_data, | ||
'user_input_field_list', |
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 provided code snippet seems to be part of a React component handling updates for node data fields, specifically focusing on input variables. Here are some observations and suggestions:
-
Variable Initialization: The function
update_field
is defined within a context where it might not have access to all required dependencies. Ensure thatprops.nodeModel
,isWorkFlow
,ok.data.work_flow.nodes
, etc., are properly initialized. -
JSON Parsing: When dealing with stringified JSON objects (e.g.,
default_value
,value
), ensure that you handle any potential errors or exceptions during deserialization. Consider using.catch()
or type casting to manage these cases. -
Type Checking: In the logic that handles different field types (api_input_field_list and user_input_field_list), ensure that the variable names (
variable
) used in comparisons match the actual keys in the JSON structure. This could lead to bugs if they do not align. -
Null Value Handling: For labels, which can also be stringified json, consider adding checks to avoid
undefined
values when extracting the label property. -
Optimization: If the list operations involve many items, consider using more efficient methods like array spreads instead of creating new arrays at each iteration.
Here's an updated version of the code incorporating these considerations:
const update_field = () => {
return Ok.ok({
properties: [
{
// Update your payload according to your application requirements here
node_data: {
api_input_field_list: [...],
user_input_field_list: [...]
}
}
]
});
};
// Example call:
Ok.then(update_field).then((response) => {
const { node_model } = response.payload;
if (node_model) {
let merged_user_input = [...node_model.properties[node_model.name_of_node]];
// Assuming you want to merge based on certain conditions
merged_user_input.forEach(item => {
const findFieldApiList = old_api_input_field_list.find(oldItem =>
newItem.variable === item.variable
);
if (findFieldApiList) {
item.default_value = JSON.parse(JSON.stringify(findFieldApiList.default_value));
if ('value' in findFieldApiList && typeof findFieldApiList.value !== 'string') {
try {
item.value = JSON.parse(JSON.stringify(findFieldApiList.value)); // Add error handling
} catch (error) {
console.error('Error parsing value:', error);
}
}
if (
(typeof item.label === 'object' || typeof item.label === 'array') &&
item.label?.label !== undefined
) {
item.label = item.label.label; // Extract label from object or array
}
} else {
item.default_value = '';
item.value = ''; // Initialize or set appropriate defaults
}
// Merge similarly for other lists...
});
node_model.properties[node_model.name_of_node] = merged_user_input;
console.log(node_model); // Log the updated model
} else {
console.error("Node model was not found.");
}
}).catch(error => {
console.error('There was a problem updating fields!', error.message);
});
Key Changes:
- Added initial checks for
old_api_input_field_list
andnode_model
. - Ensured proper handling of
JSON.stringify
to prevent circular references. - Applied similar checks and treatments to both
api_input_field_list
anduser_input_field_list
. - Included basic error handling for
default_value
parsing. - Used logical operators correctly and ensured consistent naming conventions.
fix: fix github#1988 --bug=1051321 --user=王孝刚 【github#1988】【应用】应用编排接入带用户输入参数的应用,选择后,再次进入应用,选择的参数消失了 https://www.tapd.cn/57709429/s/1643562 --bug=1051323 --user=王孝刚 【应用】创建应用报错 https://www.tapd.cn/57709429/s/1643571