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

handle approve/reject states and rejection comments #2

Open
wants to merge 10 commits into
base: aem650
Choose a base branch
from

Conversation

tgallant
Copy link
Collaborator

No description provided.

Copy link

@hanibhat hanibhat left a comment

Choose a reason for hiding this comment

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

I had a look and added some comments. Maybe someone else can also look at this, because I'm not super experienced in Java.


throw new TranslationException("This function is not implemented",
TranslationException.ErrorCode.SERVICE_NOT_IMPLEMENTED);
Comment comment) throws TranslationException {

Choose a reason for hiding this comment

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

Is the margin change intentional?

log.trace("BootstrapTranslationServiceImpl.addTranslationObjectComment");
String annotation = comment.getAnnotationData();
String author = comment.getAuthorName();
String fileName = String.format("%s_%s.txt");

Choose a reason for hiding this comment

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

The specifiers are missing. Did you mean to pass annotation and author?

@@ -325,13 +326,13 @@ public TranslationStatus getTranslationJobStatus(String strTranslationJobID) thr

@Override
public CommentCollection<Comment> getTranslationJobCommentCollection(String strTranslationJobID) {
log.trace("BootstrapTranslationServiceImpl.getTranslationJobCommentCollection");

Choose a reason for hiding this comment

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

Why warn instead of trace?

@@ -473,14 +481,25 @@ public TranslationStatus getTranslationObjectStatus(String strTranslationJobID,
if (file.labels.contains("approval=REJECTED")) {
rejected++;
}
if (file.labels.contains("approval=COMPLETE")) {
rejected++;

Choose a reason for hiding this comment

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

did you mean complete?

if (hasImported && hasTranslated && approved >= imported) {
log.warn("lilt: status for {} is APPROVED", objectPath);
return TranslationStatus.APPROVED;
}
if (hasImported && hasTranslated && translated > rejected) {
log.warn("lilt: status for {} is APPROVED", objectPath);

Choose a reason for hiding this comment

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

Shouldn't the log message say "... TRANSLATED"?

Also, what's the difference between this check and the one few lines below: if (hasImported && hasTranslated)?

return TranslationStatus.APPROVED;
}
if (hasImported && hasTranslated && hasRejected) {
log.info("lilt: job status for {} is TRANSLATION_IN_PROGRESS", strTranslationJobID);

Choose a reason for hiding this comment

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

Did you mean to warn here instead of info?
You should probably log REJECTED status instead of TRANSLATION_IN_PROGRESS and return that too.

@@ -397,23 +419,53 @@ public String uploadTranslationObject(String strTranslationJobID, TranslationObj

@Override
public TranslationStatus updateTranslationObjectState(String strTranslationJobID,
TranslationObject translationObject, TranslationState state) throws TranslationException {

Choose a reason for hiding this comment

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

Margin change.

return TranslationStatus.TRANSLATED;
}
if (hasImported) {
log.warn("lilt: status for {} is TRANSLATION_IN_PROGRESS", objectPath);

Choose a reason for hiding this comment

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

Shouldn't these types of log messages be info?

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.

2 participants