Skip to content

feat: Certificate list supports search by name and hides specified co…#8410

Merged
f2c-ci-robot[bot] merged 1 commit intodev-v2from
pr@dev-v2@common
Apr 16, 2025
Merged

feat: Certificate list supports search by name and hides specified co…#8410
f2c-ci-robot[bot] merged 1 commit intodev-v2from
pr@dev-v2@common

Conversation

@zhengkunwang223
Copy link
Copy Markdown
Member

@zhengkunwang223 zhengkunwang223 commented Apr 16, 2025

Refs #7550

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented Apr 16, 2025

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.

Details

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.

searchSSL(request)
.then((res) => {
data.value = res.data.items || [];
paginationConfig.total = res.data.total;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here are some observations and suggestions regarding the code:

  1. Empty Array Initialization: The columns array is initialized before being populated with values (ref([])), but this initialization might not be necessary unless it's intended to hold predefined column definitions.

  2. Conditional Check in Template Slot for SSL Log Button: In the table main slot, there's a conditional check for row.provider != 'custom'. If this condition is true, the custom icon will be displayed; otherwise, only regular buttons will appear. It would be better if the logic was simplified or if additional information about row.provider could be provided.

  3. Code Duplication: There’s some repetition around defining columns within different parts of the template. Reusing variables like props or using functions that populate arrays can help avoid code duplication.

  4. Comments on Columns: Comments near each <el-table-column> define what they represent (e.g., "website.domain"). Consider updating these comments based on new translations or changes in data structure.

  5. Inline Templates: Inline templates inside slots such as #default are generally concise and work well when used sparingly. However, consider whether more complex templating needs could benefit from extracting those components into separate reusable Vue components.

  6. Reactive Object Properties: The properties under req (domain, etc.), seem reactive since they are declared with reactive(). Ensure that updates made through one property cause re-renders where needed without inadvertently triggering unnecessary computations elsewhere in the component.

  7. Function Naming Consistency: Function names like getProvider, are prefixed differently between usage (uppercase vs. camel case). This may lead to confusion or inconsistencies, especially if similar function types exist later on.

  8. Data Loading Logic: The use of loading.value to manage loading indicators is clean and straightforward. Make sure that appropriate error handling scenarios have been accounted for (like network errors).

Overall, you're making good progress and most elements look reasonable given their current complexity and functionality requirements. Here's an optimized version focusing on reducing redundancy, improving readability, and ensuring consistency across your component:

// Simplified Column Definition in Script Block
const defaultColumns = [
    { label: $t('website.domain'), prop: 'primaryDomain', min_width: 150 },
    // Add other common columns here
];
columns.value = defaultColumns;

function searchSSL(params) {
    const request = {
        page: paginationConfig.currentPage,
        pageSize: paginationConfig.pageSize,
        ...params,
    };
    
    fetch(sslApiUrl, {
        method: 'GET',
        headers: apiHeaders,
        body: JSON.stringify(request),
    })
        .then(response => response.json())
        .then(res => {
            console.log(res);
            if (!res.success) throw Error(`Error fetching ssl list`);
            
            data.value = res.data.items || [];
            paginationConfig.total = res.data.total;
        })
        .catch(error => {
            loading.value = false;
            handleError();
        });
}

fetchCaList(searchParams)
.then(data => {
...
});

obtainCertification({ domains, email }) 
...
};

By addressing these areas, your code should become cleaner and potentially more efficient while retaining its existing behavior and functionalities. Always validate performance impact and adjust accordingly during development.

total, sslList, err := websiteSSLRepo.Page(search.Page, search.PageSize, opts...)
if err != nil {
return 0, nil, err
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code does not contain any significant irregularities or major issues, but there are a few optimizations and improvements that could be made:

  1. Consistent Interface: Ensure that Page method's return types and parameters follow the same consistency with other methods in the WebsiteSSLService. This can make the interface more intuitive.

  2. Null Domain Handling: Consider adding null checks for the search.Domain before appending it to opts. If the domain is empty, you might want to skip this option rather than filtering directly which can improve performance.

if search.Domain == "" {
    // Skip this filter if the domain is not provided
} else {
    opts = append(opts, websiteSSLRepo.WithByDomain(search.Domain))
}
  1. Variable Declaration: You can declare all variables at the beginning of the method to reduce cognitive complexity and improve readability.

  2. Use of Variadic Parameters: Instead of manually managing variadic arguments via slices, consider using Go's built-in variadic functions where applicable. For example:

total, sslList, err := websiteSSLRepo.Page(search.Page, search.PageSize, opts...)

This way, changes like additional sorting criteria can be easily added without modifying multiple calls to the append function.

  1. Doc Comments: Add comments explaining each step of the logic within Page method to enhance its documentation and maintainability.

  2. Error Handling Flexibility: Consider returning detailed error messages that include information about what caused the error, particularly when dealing with non-specific errors like "failed to fetch data".

Here's a slightly refactored version incorporating some of these suggestions:

func (w WebsiteSSLService) Page(search request.WebsiteSSLSearch) (int64, []response.WebsiteSSLDTO, error) {
	opts := []repo.DBOption{
		repo.WithOrderBy("created_at desc"),
	}

	if search.Domain != "" {
		opts = append(opts, websiteSSLRepo.WithByDomain(search.Domain))
	}

	total, sslList, err := websiteSSLRepo.PageWithOptions(search.Page, search.PageSize, opts...)

	if err != nil && total > 0 {
		err = fmt.Errorf("%d records returned while fetching SSL data failed: %v", total, err)
	}

	return total, sslList, err
}

These suggestions will help make the Page method more robust, readable, and maintainable.

}

func (w WebsiteSSLRepo) Page(page, size int, opts ...DBOption) (int64, []model.WebsiteSSL, error) {
var sslList []model.WebsiteSSL
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a code snippet from an interface (ISSLRepo) and its implementations for querying SSL websites based on various conditions. The changes made include adding a new method WithByDomain to allow filtering by domain name, modifying the existing Page function slightly to incorporate pagination parameters, and implementing error handling where necessary.

Irregularities/Issues:

  • Error Handling for Pagination: The Page function currently returns two values (the total count and a slice of models). While this pattern can be useful, it might not work correctly if errors occur during database operations. Consider wrapping these in a single return value with custom types that handle both data and potential errors.

Optimization Suggestions:

  1. Indexing: If using SQLite, consider creating indexes on columns used frequently in WHERE clauses like acme_account_id, dns_account_id, ca_id, and primary_domain. This can significantly speed up queries.

  2. Consistency in Functionality: Ensure consistency across all filter functions regarding whether they return the original model or a modified model with additional fields. For example, use the same approach for returning the "first" model as you do for paginated results.

  3. Performance Tuning: Depending on the complexity of your query and usage patterns, consider optimizing SQL performance by analyzing slow queries, rewriting complex queries into simpler ones, or caching common query results.

  4. Refactoring for Readability: Break down larger functions or methods into smaller, more focused parts if they become difficult to manage visually. This improves readability and maintainability.

These suggestions aim to improve the robustness, efficiency, and overall quality of the codebase managing SSL website records.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

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

/lgtm

@wanghe-fit2cloud
Copy link
Copy Markdown
Member

/approve

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented Apr 16, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot Bot merged commit b520b74 into dev-v2 Apr 16, 2025
6 checks passed
@f2c-ci-robot f2c-ci-robot Bot deleted the pr@dev-v2@common branch April 16, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants