From 14ee106698e91af4249feb0dde0445de4284a3d3 Mon Sep 17 00:00:00 2001 From: Harold Wanyama Date: Thu, 25 May 2023 18:10:35 +0300 Subject: [PATCH] [#3936] Bug Update Approval List - Resolved dynamo query when updating approval list previously having wrong queries for existing eclas Signed-off-by: Harold Wanyama --- cla-backend-go/signatures/repository.go | 110 +++++++++----- cla-backend-go/signatures/service.go | 185 +++++++++++++++++------- 2 files changed, 213 insertions(+), 82 deletions(-) diff --git a/cla-backend-go/signatures/repository.go b/cla-backend-go/signatures/repository.go index 01cfd17b6..468fd1628 100644 --- a/cla-backend-go/signatures/repository.go +++ b/cla-backend-go/signatures/repository.go @@ -80,7 +80,7 @@ type SignatureRepository interface { GetProjectCompanySignature(ctx context.Context, companyID, projectID string, approved, signed *bool, nextKey *string, pageSize *int64) (*models.Signature, error) GetProjectCompanySignatures(ctx context.Context, companyID, projectID string, approved, signed *bool, nextKey *string, sortOrder *string, pageSize *int64) (*models.Signatures, error) GetProjectCompanyEmployeeSignatures(ctx context.Context, params signatures.GetProjectCompanyEmployeeSignaturesParams, criteria *ApprovalCriteria) (*models.Signatures, error) - GetProjectCompanyEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, employeeUserModel *models.User) (*models.Signature, error) + GetProjectCompanyEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, employeeUserModel *models.User, wg *sync.WaitGroup, resultChannel chan<- *EmployeeModel, errorChannel chan<- error) CreateProjectCompanyEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, employeeUserModel *models.User) error GetCompanySignatures(ctx context.Context, params signatures.GetCompanySignaturesParams, pageSize int64, loadACL bool) (*models.Signatures, error) GetCompanyIDsWithSignedCorporateSignatures(ctx context.Context, claGroupID string) ([]SignatureCompanyID, error) @@ -1792,7 +1792,14 @@ func getLatestSignatures(signatures []*models.Signature) []*models.Signature { return result } -func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, employeeUserModel *models.User) (*models.Signature, error) { +type EmployeeModel struct { + Signature *models.Signature + User *models.User +} + +func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, employeeUserModel *models.User, wg *sync.WaitGroup, resultChannel chan<- *EmployeeModel, errorChannel chan<- error) { + defer wg.Done() + f := logrus.Fields{ "functionName": "v1.signatures.repository.GetProjectCompanyEmployeeSignature", utils.XREQUESTID: ctx.Value(utils.XREQUESTID), @@ -1812,7 +1819,8 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c } if companyModel == nil || claGroupModel == nil || employeeUserModel == nil { - return nil, nil + resultChannel <- nil + return } // This is the keys we want to match @@ -1832,7 +1840,8 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c if err != nil { log.WithFields(f).WithError(err).Warnf("error building expression for employee signature query, company model: %+v, CLA group model: %+v, employee model: %+v", companyModel, claGroupModel, employeeUserModel) - return nil, err + errorChannel <- err + return } // Assemble the query input parameters @@ -1852,10 +1861,15 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c if errQuery != nil { log.WithFields(f).WithError(errQuery).Warnf("error retrieving project company employee acknowledgement record for company model: %+v, CLA group model: %+v, employee model: %+v", companyModel, claGroupModel, employeeUserModel) - return nil, errQuery + errorChannel <- errQuery + return } if results == nil || len(results.Items) == 0 { - return nil, nil + resultChannel <- &EmployeeModel{ + Signature: nil, + User: employeeUserModel, + } + return } log.WithFields(f).Debugf("returned %d results", len(results.Items)) // Convert the list of DB models to a list of response models @@ -1863,11 +1877,13 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c if modelErr != nil { log.WithFields(f).WithError(modelErr).Warnf("error converting DB model to response model for project company employee acknowledgement record for company model: %+v, CLA group model: %+v, employee model: %+v", companyModel, claGroupModel, employeeUserModel) - return nil, modelErr + errorChannel <- modelErr + return } if len(signatureList) == 0 { - return nil, nil + resultChannel <- nil + return } if len(signatureList) > 1 { @@ -1875,7 +1891,10 @@ func (repo repository) GetProjectCompanyEmployeeSignature(ctx context.Context, c companyModel, claGroupModel, employeeUserModel) } - return signatureList[0], nil + resultChannel <- &EmployeeModel{ + Signature: signatureList[0], + User: employeeUserModel, + } } // CreateProjectCompanyEmployeeSignature creates a new project employee signature using the provided details @@ -1898,25 +1917,46 @@ func (repo repository) CreateProjectCompanyEmployeeSignature(ctx context.Context f["employeeEmails"] = strings.Join(employeeUserModel.Emails, ",") } - existingSig, lookupErr := repo.GetProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, employeeUserModel) - if lookupErr != nil { - log.WithFields(f).WithError(lookupErr).Warnf("problem looking up existing signature for project: %+v, company: %+v, employee: %+v", claGroupModel, companyModel, employeeUserModel) - } + var wg sync.WaitGroup + resultChan := make(chan *EmployeeModel) + errorChan := make(chan error) + + wg.Add(1) + go repo.GetProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, employeeUserModel, &wg, resultChan, errorChan) + + go func() { + wg.Wait() + close(resultChan) + close(errorChan) + }() + + for result := range resultChan { + if result != nil { + existingSig := result.Signature + // If exists, need to update + if existingSig != nil { + log.WithFields(f).Debug("found existing employee acknowledgement") + if !existingSig.SignatureApproved { + log.WithFields(f).Debugf("found existing employee acknowledgement, but not currently approved.") + validateRecordErr := repo.ValidateProjectRecord(ctx, existingSig.SignatureID, fmt.Sprintf(" Enabled previously disabled employee acknowledgement via CLA Manager approval list edit with auto-enable feature flag configured on %s.", utils.CurrentSimpleDateTimeString())) + if validateRecordErr != nil { + return validateRecordErr + } + return nil + } - // If exists, need to update - if existingSig != nil { - log.WithFields(f).Debug("found existing employee acknowledgement") - if !existingSig.SignatureApproved { - log.WithFields(f).Debugf("found existing employee acknowledgement, but not currently approved.") - validateRecordErr := repo.ValidateProjectRecord(ctx, existingSig.SignatureID, fmt.Sprintf(" Enabled previously disabled employee acknowledgement via CLA Manager approval list edit with auto-enable feature flag configured on %s.", utils.CurrentSimpleDateTimeString())) - if validateRecordErr != nil { - return validateRecordErr + return nil } - return nil } + } - return nil + for err := range errorChan { + if err != nil { + log.WithFields(f).WithError(err).Warnf("error creating project company employee signature for company model: %+v, CLA group model: %+v, employee model: %+v", + companyModel, claGroupModel, employeeUserModel) + } } + log.WithFields(f).Debugf("creating project company employee signature for project: %+v, company: %+v, employee: %+v", claGroupModel, companyModel, employeeUserModel) // If not exists, need to create @@ -2016,10 +2056,6 @@ func (repo repository) getProjectCompanyEmployeeSignatureCount(ctx context.Conte var filterAdded bool var filter expression.ConditionBuilder - // Check for approved signatures - filter = addAndCondition(filter, expression.Name("signature_approved").Equal(expression.Value(true)), &filterAdded) - filter = addAndCondition(filter, expression.Name("signature_signed").Equal(expression.Value(true)), &filterAdded) - if criteria != nil && criteria.GitHubUsername != "" { filter = addAndCondition(filter, expression.Name(SignatureUserGitHubUsername).Equal(expression.Value(criteria.GitHubUsername)), &filterAdded) } @@ -2035,7 +2071,11 @@ func (repo repository) getProjectCompanyEmployeeSignatureCount(ctx context.Conte beforeQuery, _ := utils.CurrentTime() log.WithFields(f).Debugf("running total signature count query on table: %s", repo.signatureTableName) // Use the nice builder to create the expression - expr, err := expression.NewBuilder().WithKeyCondition(condition).WithFilter(filter).WithProjection(buildCountProjection()).Build() + expressionBuilder := expression.NewBuilder().WithKeyCondition(condition).WithProjection(buildCountProjection()) + if filterAdded { + expressionBuilder = expressionBuilder.WithFilter(filter) + } + expr, err := expressionBuilder.Build() if err != nil { log.WithFields(f).Warnf("error building expression for project signature ID query, project: %s, error: %v", params.ProjectID, err) @@ -2048,11 +2088,15 @@ func (repo repository) getProjectCompanyEmployeeSignatureCount(ctx context.Conte ExpressionAttributeNames: expr.Names(), ExpressionAttributeValues: expr.Values(), KeyConditionExpression: expr.KeyCondition(), - FilterExpression: expr.Filter(), - ProjectionExpression: expr.Projection(), - TableName: aws.String(repo.signatureTableName), - IndexName: aws.String("signature-user-ccla-company-index"), // Name of a secondary index to scan - Limit: aws.Int64(pageSize), + // FilterExpression: expr.Filter(), + ProjectionExpression: expr.Projection(), + TableName: aws.String(repo.signatureTableName), + IndexName: aws.String("signature-user-ccla-company-index"), // Name of a secondary index to scan + Limit: aws.Int64(pageSize), + } + + if filterAdded { + queryInput.FilterExpression = expr.Filter() } var lastEvaluatedKey string diff --git a/cla-backend-go/signatures/service.go b/cla-backend-go/signatures/service.go index 89a56ea75..4e96e50f2 100644 --- a/cla-backend-go/signatures/service.go +++ b/cla-backend-go/signatures/service.go @@ -771,39 +771,82 @@ func (s service) CreateOrUpdateEmployeeSignature(ctx context.Context, claGroupMo log.WithFields(f).WithError(userErr).Warnf("problem creating or loading user records from the approval list") } + responseErr := s.processEmployeeSignatures(ctx, companyModel, claGroupModel, userList) + + if responseErr != nil { + log.WithFields(f).WithError(responseErr).Warnf("problem processing employee signatures") + } + + return userList, responseErr +} + +func (s service) processEmployeeSignatures(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, userList []*models.User) error { + f := logrus.Fields{ + "functionName": "v2.company.service.processEmployeeSignatures", + utils.XREQUESTID: ctx.Value(utils.XREQUESTID), + "claGroupID": claGroupModel.ProjectID, + "companyName": companyModel.CompanyName, + "companyID": companyModel.CompanyID, + } + var responseErr error + var wg sync.WaitGroup + resultChan := make(chan *EmployeeModel) + errChan := make(chan error) // For each item in the email approval list... for _, employeeUserModel := range userList { + wg.Add(1) + go s.repo.GetProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, employeeUserModel, &wg, resultChan, errChan) + } - // Look up the employee signature record - it may not exist - employeeSignatureModel, employeeSignatureErr := s.repo.GetProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, employeeUserModel) - if employeeSignatureErr != nil { - log.WithFields(f).WithError(employeeSignatureErr).Warnf("unexpected problem looking up employee signature record for: %+v - unable to process new employee acknowledgement request", employeeUserModel) - responseErr = employeeSignatureErr - continue - } + // Wait for all the go routines to complete + go func() { + wg.Wait() + close(resultChan) + close(errChan) + }() - if employeeSignatureModel != nil { - if !employeeSignatureModel.SignatureApproved || !employeeSignatureModel.SignatureSigned { - // If record exists, this will update the record - updateErr := s.repo.ValidateProjectRecord(ctx, employeeSignatureModel.SignatureID, "signed and approved employee acknowledgement since auto_create_ecla feature flag set to true") - if updateErr != nil { - log.WithFields(f).WithError(updateErr).Warnf("problem updating employee signature record for: %+v", employeeUserModel) - responseErr = updateErr + for employeeModel := range resultChan { + if employeeModel != nil { + employeeSignatureModel := employeeModel.Signature + employeeUserModel := employeeModel.User + log.WithFields(f).Debugf("processing employee signature record for user: %+s", employeeUserModel.UserID) + if employeeSignatureModel != nil { + if !employeeSignatureModel.SignatureApproved || !employeeSignatureModel.SignatureSigned { + // If record exists, this will update the record + log.WithFields(f).Debugf("updating employee signature record for: %+v", employeeSignatureModel) + updateErr := s.repo.ValidateProjectRecord(ctx, employeeSignatureModel.SignatureID, "signed and approved employee acknowledgement since auto_create_ecla feature flag set to true") + if updateErr != nil { + log.WithFields(f).WithError(updateErr).Warnf("problem updating employee signature record for: %+v", employeeSignatureModel) + responseErr = updateErr + } + } else { + log.WithFields(f).Debugf("employee signature record already exists for: %+v", employeeUserModel) + } + } else { + // Ok, auto-create the employee acknowledgement record + log.WithFields(f).Debugf("creating employee signature record for user: %+s", employeeUserModel.UserID) + createErr := s.repo.CreateProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, employeeUserModel) + if createErr != nil { + log.WithFields(f).WithError(createErr).Warnf("unable to create project company employee signature record for: %+v", employeeUserModel) + responseErr = createErr } - } - } else { - // Ok, auto-create the employee acknowledgement record - createErr := s.repo.CreateProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, employeeUserModel) - if createErr != nil { - log.WithFields(f).WithError(createErr).Warnf("unable to create project company employee signature record for: %+v", employeeUserModel) - responseErr = createErr } } } - return userList, responseErr + // Check for any errors + for err := range errChan { + if err != nil { + log.WithFields(f).WithError(err).Warnf("problem looking up employee signature record ") + responseErr = err + } + } + + log.WithFields(f).Debugf("completed processing employee signatures") + + return responseErr } // InvalidateProjectRecords disassociates project signatures @@ -1093,44 +1136,88 @@ func (s service) hasUserSigned(ctx context.Context, user *models.User, projectID return &hasSigned, &companyAffiliation, claGroupModelErr } - // Load the user's employee acknowledgement - make sure it is valid - employeeSignature, empSigErr := s.repo.GetProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, user) - if empSigErr != nil { - log.WithFields(f).WithError(empSigErr).Warnf("problem looking up employee signature for user: %s, company: %s, project: %s", user.UserID, companyID, projectID) - return &hasSigned, &companyAffiliation, empSigErr + employeeSigned, err := s.processEmployeeSignature(ctx, companyModel, claGroupModel, user) + + if err != nil { + log.WithFields(f).WithError(err).Warnf("problem looking up employee signature for company: %s", companyID) + return &hasSigned, &companyAffiliation, err + } + if employeeSigned != nil { + hasSigned = *employeeSigned } - if employeeSignature != nil { - log.WithFields(f).Debugf("ECLA Signature check - located employee acknowledgement - signature id: %s", employeeSignature.SignatureID) + } else { + log.WithFields(f).Debugf("ECLA signature check - user does not have a company ID assigned - skipping...") + } - // Get corporate ccla signature of company to access the approval list - cclaSignature, cclaErr := s.GetCorporateSignature(ctx, projectID, companyID, &approved, &signed) - if cclaErr != nil { - log.WithFields(f).WithError(cclaErr).Warnf("problem looking up ECLA signature for company: %s, project: %s", companyID, projectID) - return &hasSigned, &companyAffiliation, cclaErr - } + return &hasSigned, &companyAffiliation, nil +} + +func (s service) processEmployeeSignature(ctx context.Context, companyModel *models.Company, claGroupModel *models.ClaGroup, user *models.User) (*bool, error) { + f := logrus.Fields{ + "functionName": "v2.signatures.service.processEmployeeSignature", + utils.XREQUESTID: ctx.Value(utils.XREQUESTID), + "companyID": companyModel.CompanyID, + "projectID": claGroupModel.ProjectID, + "userID": user.UserID, + } + var wg sync.WaitGroup + resultChannel := make(chan *EmployeeModel) + errorChannel := make(chan error) + hasSigned := false + projectID := claGroupModel.ProjectID + companyID := companyModel.CompanyID + approved := true + signed := true + + wg.Add(1) + s.repo.GetProjectCompanyEmployeeSignature(ctx, companyModel, claGroupModel, user, &wg, resultChannel, errorChannel) + + go func() { + wg.Wait() + close(resultChannel) + close(errorChannel) + }() - if cclaSignature != nil { - userApproved, approvedErr := s.userIsApproved(ctx, user, cclaSignature) - if approvedErr != nil { - log.WithFields(f).WithError(approvedErr).Warnf("problem determining if user: %s is approved for project: %s", user.UserID, projectID) - return &hasSigned, &companyAffiliation, approvedErr + for result := range resultChannel { + if result != nil { + employeeSignature := result.Signature + if employeeSignature != nil { + log.WithFields(f).Debugf("ECLA Signature check - located employee acknowledgement - signature id: %s", employeeSignature.SignatureID) + + // Get corporate ccla signature of company to access the approval list + cclaSignature, cclaErr := s.GetCorporateSignature(ctx, projectID, companyID, &approved, &signed) + if cclaErr != nil { + log.WithFields(f).WithError(cclaErr).Warnf("problem looking up ECLA signature for company: %s, project: %s", companyID, projectID) + return &hasSigned, cclaErr } - log.WithFields(f).Debugf("ECLA Signature check - user approved: %t for projectID: %s for company: %s", userApproved, projectID, user.CompanyID) - if userApproved { - log.WithFields(f).Debugf("user: %s is in the approval list for signature: %s", user.UserID, employeeSignature.SignatureID) - hasSigned = true + if cclaSignature != nil { + userApproved, approvedErr := s.userIsApproved(ctx, user, cclaSignature) + if approvedErr != nil { + log.WithFields(f).WithError(approvedErr).Warnf("problem determining if user: %s is approved for project: %s", user.UserID, projectID) + return &hasSigned, approvedErr + } + log.WithFields(f).Debugf("ECLA Signature check - user approved: %t for projectID: %s for company: %s", userApproved, projectID, user.CompanyID) + + if userApproved { + log.WithFields(f).Debugf("user: %s is in the approval list for signature: %s", user.UserID, employeeSignature.SignatureID) + hasSigned = true + } } + } else { + log.WithFields(f).Debugf("ECLA Signature check - unable to locate employee acknowledgement for user: %s, company: %s, project: %s", user.UserID, companyID, projectID) } - } else { - log.WithFields(f).Debugf("ECLA Signature check - unable to locate employee acknowledgement for user: %s, company: %s, project: %s", user.UserID, companyID, projectID) } - } else { - log.WithFields(f).Debugf("ECLA signature check - user does not have a company ID assigned - skipping...") } - return &hasSigned, &companyAffiliation, nil + for empSigErr := range errorChannel { + log.WithFields(f).WithError(empSigErr).Warnf("problem looking up employee signature for user: %s, company: %s, project: %s", user.UserID, companyID, projectID) + return &hasSigned, empSigErr + } + + return &hasSigned, nil + } func (s service) userIsApproved(ctx context.Context, user *models.User, cclaSignature *models.Signature) (bool, error) {