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

perf: add warning msg for acl_face_verify #1611

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

fit2bot
Copy link
Contributor

@fit2bot fit2bot commented Dec 16, 2024

perf: add warning msg for acl_face_verify

@fit2bot fit2bot requested a review from a team December 16, 2024 07:27
msg := lang.T("Face verification is not supported yet. Please use the WebTerminal to connect the asset.")
utils.IgnoreErrWriteString(u.h.term, msg)
utils.IgnoreErrWriteString(u.h.term, utils.CharNewLine)
return
case model.ACLReview:
reviewHandler := LoginReviewHandler{
readWriter: u.h.sess,
Copy link
Member

Choose a reason for hiding this comment

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

None found.

Regarding knowledge cutoff, we're currently at February 28th of the year 2024 so there should be no such problems.

msg := lang.T("Face verification is not supported yet. Please use the WebTerminal to connect the asset.")
utils.IgnoreErrWriteString(d.term, msg)
utils.IgnoreErrWriteString(d.term, utils.CharNewLine)
return
case model.ACLReview:
reviewHandler := LoginReviewHandler{
readWriter: d.wrapperSess,
Copy link
Member

Choose a reason for hiding this comment

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

In the given source code snippet, some key lines that may indicate potential issues include the use of "utils.IgnoreErrWriteString" which seems incorrect because error handling using this function often involves logging rather than writing strings back out.

It is recommended to review these specific functions:

  • ignoreError() and their implementations should correctly log errors instead of simply ignoring them.

  • Check whether ignoreError is part of or imported from necessary packages like log for proper context and functionality. It could be causing unnecessary overhead if it's used with other non-standard functions that don't need such logging behavior.

Additionally, I noticed that there might possibly be a syntax/mistake on line 446 in the case structure. The keyword was incorrectly mentioned twice as an argument. Here's the corrected version assuming the intention behind the keywords were correct:


case model.ACLReview:
   // todo: Reviewing the content and asking users for feedback...
   logger.Errorf("Failed to create connect token and auth information after reviewing: %s %s", tokenInfo.Code, tokenInfo.Detail)
   msg := fmt.Sprintf(lang.T("Reviewing asset content..."), asset.PermStr())
   utils.WriteStringToConsole(d.term, msg)
   return

If the purpose of checking the review status against different statuses wasn’t intended, the logic should be adjusted accordingly within the respective cases. This ensures clarity, precision, and avoids unexpected behaviors during debug analysis.

@@ -69,6 +69,8 @@ type ConnectTokenInfo struct {
const (
ACLReview = "acl_review"
ACLReject = "acl_reject"

ACLFaceVerify = "acl_face_verify"
)

type ConnectOptions struct {
Copy link
Member

Choose a reason for hiding this comment

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

The code seems to be correct and contains no apparent irregularities, issues or optimizations needed at this moment.

@LeeEirc LeeEirc merged commit 6c8349d into dev Dec 16, 2024
6 of 7 checks passed
@LeeEirc LeeEirc deleted the pr@dev@perf_acl_verify branch December 16, 2024 07:28
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