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.

Sign up for GitHub

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

Feature ETP: Add Cloning hook for Assistants and KBFile #58

Closed
wants to merge 1 commit into from
Closed

Feature ETP: Add Cloning hook for Assistants and KBFile #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src-db/database/sourcedata/AD_COLUMN.xml
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

GPT Review for AD_COLUMN.xml

Review

  • Estimated effort to review [1-5]:
    1, because the change is minimal and straightforward, involving only a single line modification in an XML file.
  • Score: 95

Code feedback

  • File:
    src-db/database/sourcedata/AD_COLUMN.xml
  • Language:
    xml
  • Suggestion:
    Ensure that the change in the IS_CHILD_PROPERTY_IN_PARENT field from N to Y is intentional and aligns with the business logic or requirements. This change could have implications on how child properties are handled in parent entities. [important]
  • Label:
    possible issue
  • Existing code:
<!--D81008C38D71461E8678EC984B1A8748-->  <IS_CHILD_PROPERTY_IN_PARENT><![CDATA[N]]></IS_CHILD_PROPERTY_IN_PARENT>
  • Improved code:
<!--D81008C38D71461E8678EC984B1A8748-->  <IS_CHILD_PROPERTY_IN_PARENT><![CDATA[Y]]></IS_CHILD_PROPERTY_IN_PARENT>

Original file line number Diff line number Diff line change
Expand Up @@ -5841,7 +5841,7 @@ Temperature adjusts the probability distribution for selecting the next word in
<!--D81008C38D71461E8678EC984B1A8748--> <ALLOWSORTING><![CDATA[Y]]></ALLOWSORTING>
<!--D81008C38D71461E8678EC984B1A8748--> <ALLOWFILTERING><![CDATA[Y]]></ALLOWFILTERING>
<!--D81008C38D71461E8678EC984B1A8748--> <ALLOWED_CROSS_ORG_LINK><![CDATA[N]]></ALLOWED_CROSS_ORG_LINK>
<!--D81008C38D71461E8678EC984B1A8748--> <IS_CHILD_PROPERTY_IN_PARENT><![CDATA[N]]></IS_CHILD_PROPERTY_IN_PARENT>
<!--D81008C38D71461E8678EC984B1A8748--> <IS_CHILD_PROPERTY_IN_PARENT><![CDATA[Y]]></IS_CHILD_PROPERTY_IN_PARENT>
<!--D81008C38D71461E8678EC984B1A8748--></AD_COLUMN>

<!--D8203893352D417290621B6834D6E6A7--><AD_COLUMN>
Expand Down
8 changes: 4 additions & 4 deletions src-db/database/sourcedata/AD_TAB.xml
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

GPT Review for AD_TAB.xml

Review

  • Estimated effort to review [1-5]:
    1, because the changes are straightforward and involve simple modifications to XML values. The changes are easy to understand and verify.
  • Score: 95

Code feedback

  • File:
    src-db/database/sourcedata/AD_TAB.xml
  • Language:
    xml
  • Suggestion:
    Ensure that the changes in the XML values align with the intended functionality and requirements. Specifically, verify that enabling the EM_OBUIAPP_SHOW_CLONE_BUTTON and disabling EM_OBUIAPP_CLONE_CHILDREN is the desired behavior for the application. [important]
  • Label:
    best practice
  • Existing code:
<!--09F802E423924081BC2947A64DDB5AF5-->  <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[Y]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--09F802E423924081BC2947A64DDB5AF5-->  <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[N]]></EM_OBUIAPP_CLONE_CHILDREN>
  • Improved code:
<!--09F802E423924081BC2947A64DDB5AF5-->  <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[Y]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--09F802E423924081BC2947A64DDB5AF5-->  <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[N]]></EM_OBUIAPP_CLONE_CHILDREN>
  • File:
    src-db/database/sourcedata/AD_TAB.xml
  • Language:
    xml
  • Suggestion:
    Check for consistency across similar XML entries to ensure that the changes made are consistent with other parts of the XML file, especially if there are multiple entries for AD_TAB. [medium]
  • Label:
    consistency
  • Existing code:
<!--F0AE228DDA0D4A3F98A08B8284EF1689-->  <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[Y]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--F0AE228DDA0D4A3F98A08B8284EF1689-->  <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[N]]></EM_OBUIAPP_CLONE_CHILDREN>
  • Improved code:
<!--F0AE228DDA0D4A3F98A08B8284EF1689-->  <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[Y]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--F0AE228DDA0D4A3F98A08B8284EF1689-->  <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[N]]></EM_OBUIAPP_CLONE_CHILDREN>

Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
<!--09F802E423924081BC2947A64DDB5AF5--> <EM_OBUIAPP_CAN_ADD><![CDATA[N]]></EM_OBUIAPP_CAN_ADD>
<!--09F802E423924081BC2947A64DDB5AF5--> <EM_OBUIAPP_CAN_DELETE><![CDATA[N]]></EM_OBUIAPP_CAN_DELETE>
<!--09F802E423924081BC2947A64DDB5AF5--> <EM_OBUIAPP_SHOW_SELECT><![CDATA[Y]]></EM_OBUIAPP_SHOW_SELECT>
<!--09F802E423924081BC2947A64DDB5AF5--> <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[N]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--09F802E423924081BC2947A64DDB5AF5--> <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[Y]]></EM_OBUIAPP_CLONE_CHILDREN>
<!--09F802E423924081BC2947A64DDB5AF5--> <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[Y]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--09F802E423924081BC2947A64DDB5AF5--> <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[N]]></EM_OBUIAPP_CLONE_CHILDREN>
<!--09F802E423924081BC2947A64DDB5AF5--></AD_TAB>

<!--2A20AEA4B93647C592E8FF0A1C4C7061--><AD_TAB>
Expand Down Expand Up @@ -409,8 +409,8 @@
<!--F0AE228DDA0D4A3F98A08B8284EF1689--> <EM_OBUIAPP_CAN_ADD><![CDATA[N]]></EM_OBUIAPP_CAN_ADD>
<!--F0AE228DDA0D4A3F98A08B8284EF1689--> <EM_OBUIAPP_CAN_DELETE><![CDATA[N]]></EM_OBUIAPP_CAN_DELETE>
<!--F0AE228DDA0D4A3F98A08B8284EF1689--> <EM_OBUIAPP_SHOW_SELECT><![CDATA[Y]]></EM_OBUIAPP_SHOW_SELECT>
<!--F0AE228DDA0D4A3F98A08B8284EF1689--> <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[N]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--F0AE228DDA0D4A3F98A08B8284EF1689--> <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[Y]]></EM_OBUIAPP_CLONE_CHILDREN>
<!--F0AE228DDA0D4A3F98A08B8284EF1689--> <EM_OBUIAPP_SHOW_CLONE_BUTTON><![CDATA[Y]]></EM_OBUIAPP_SHOW_CLONE_BUTTON>
<!--F0AE228DDA0D4A3F98A08B8284EF1689--> <EM_OBUIAPP_CLONE_CHILDREN><![CDATA[N]]></EM_OBUIAPP_CLONE_CHILDREN>
<!--F0AE228DDA0D4A3F98A08B8284EF1689--></AD_TAB>

</data>
68 changes: 68 additions & 0 deletions src/com/etendoerp/copilot/hook/cloning/CloneAssistant.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

GPT Review for CloneAssistant.java

Review

  • Estimated effort to review [1-5]:
    2, because the code is straightforward and well-structured, with clear method implementations. The logic is simple, focusing on cloning operations, which are easy to follow. However, a few improvements can be made to enhance code quality.
  • Score: 85

Code feedback

  • File:
    src/com/etendoerp/copilot/hook/cloning/CloneAssistant.java
  • Language:
    java
  • Suggestion:
    Consider adding error handling when saving, flushing, and refreshing the cloneAssistant object. This will ensure that any issues during these operations are caught and handled appropriately, preventing potential runtime exceptions. [important]
  • Label:
    error handling
  • Existing code:
OBDal.getInstance().save(cloneAssistant);
OBDal.getInstance().flush();
OBDal.getInstance().refresh(cloneAssistant);
return cloneAssistant;
  • Improved code:
try {
    OBDal.getInstance().save(cloneAssistant);
    OBDal.getInstance().flush();
    OBDal.getInstance().refresh(cloneAssistant);
} catch (Exception e) {
    // Log the exception and handle it appropriately
    System.err.println("Error during cloning operation: " + e.getMessage());
    // Consider rethrowing the exception or handling it based on your application's needs
}
return cloneAssistant;
  • File:
    src/com/etendoerp/copilot/hook/cloning/CloneAssistant.java
  • Language:
    java
  • Suggestion:
    Use a logger instead of System.err.println for logging errors. This is a best practice for better control over logging levels and outputs. [medium]
  • Label:
    best practice
  • Existing code:
System.err.println("Error during cloning operation: " + e.getMessage());
  • Improved code:
import java.util.logging.Logger;

private static final Logger LOGGER = Logger.getLogger(CloneAssistant.class.getName());

LOGGER.severe("Error during cloning operation: " + e.getMessage());

Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package com.etendoerp.copilot.hook.cloning;

import javax.enterprise.context.ApplicationScoped;

import org.openbravo.base.structure.BaseOBObject;
import org.openbravo.client.kernel.ComponentProvider.Qualifier;
import org.openbravo.dal.service.OBDal;

import com.etendoerp.copilot.data.CopilotApp;
import com.smf.jobs.hooks.CloneRecordHook;

@ApplicationScoped
@Qualifier(CopilotApp.ENTITY_NAME)
public class CloneAssistant extends CloneRecordHook {

/**
* Determines whether to copy child records.
* <p>
* This method always returns true, indicating that child records should be copied.
*
* @param uiCopyChildren
* A boolean indicating if the UI requests to copy children.
* @return true, indicating that child records should be copied.
*/
@Override
public boolean shouldCopyChildren(boolean uiCopyChildren) {
return true;
}

/**
* Pre-copy hook.
* <p>
* This method is called before the original record is copied. It returns the original record without modifications.
*
* @param originalRecord
* The original record to be copied.
* @return The original record.
*/
@Override
public BaseOBObject preCopy(BaseOBObject originalRecord) {
return originalRecord;
}

/**
* Post-copy hook.
* <p>
* This method is called after the original record is copied. It modifies the cloned record by setting a new name and nullifying the module.
* The cloned record is then saved, flushed, and refreshed in the database.
*
* @param originalRecord
* The original record that was copied.
* @param newRecord
* The newly created cloned record.
* @return The modified cloned record.
*/
@Override
public BaseOBObject postCopy(BaseOBObject originalRecord, BaseOBObject newRecord) {
CopilotApp originalAssistant = (CopilotApp) originalRecord;
CopilotApp cloneAssistant = (CopilotApp) newRecord;
cloneAssistant.setName("Copy of " + originalAssistant.getName());
cloneAssistant.setModule(null);

OBDal.getInstance().save(cloneAssistant);
OBDal.getInstance().flush();
OBDal.getInstance().refresh(cloneAssistant);
return cloneAssistant;
}
}
68 changes: 68 additions & 0 deletions src/com/etendoerp/copilot/hook/cloning/CloneKBFile.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

GPT Review for CloneKBFile.java

Review

  • Estimated effort to review [1-5]:
    2, because the code is straightforward and follows a clear pattern, but it involves database operations which require careful consideration.
  • Score: 85

Code feedback

  • File:
    src/com/etendoerp/copilot/hook/cloning/CloneKBFile.java
  • Language:
    java
  • Suggestion:
    Consider adding error handling around the database operations to ensure that any issues during the save, flush, or refresh processes are properly managed. This can prevent potential runtime exceptions and improve the robustness of the code. [important]
  • Label:
    error handling
  • Existing code:
OBDal.getInstance().save(cloneFile);
OBDal.getInstance().flush();
OBDal.getInstance().refresh(cloneFile);
  • Improved code:
try {
    OBDal.getInstance().save(cloneFile);
    OBDal.getInstance().flush();
    OBDal.getInstance().refresh(cloneFile);
} catch (Exception e) {
    // Log the exception and handle it appropriately
    System.err.println("Error during database operations: " + e.getMessage());
    // Consider rethrowing the exception or handling it based on your application's needs
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package com.etendoerp.copilot.hook.cloning;

import javax.enterprise.context.ApplicationScoped;

import org.openbravo.base.structure.BaseOBObject;
import org.openbravo.client.kernel.ComponentProvider.Qualifier;
import org.openbravo.dal.service.OBDal;

import com.etendoerp.copilot.data.CopilotFile;
import com.smf.jobs.hooks.CloneRecordHook;

@ApplicationScoped
@Qualifier(CopilotFile.ENTITY_NAME)
public class CloneKBFile extends CloneRecordHook {

/**
* Determines whether to copy child records.
* <p>
* This method always returns true, indicating that child records should be copied.
*
* @param uiCopyChildren
* A boolean indicating if the UI requests to copy children.
* @return true, indicating that child records should be copied.
*/
@Override
public boolean shouldCopyChildren(boolean uiCopyChildren) {
return true;
}

/**
* Pre-copy hook.
* <p>
* This method is called before the original record is copied. It returns the original record without modifications.
*
* @param originalRecord
* The original record to be copied.
* @return The original record.
*/
@Override
public BaseOBObject preCopy(BaseOBObject originalRecord) {
return originalRecord;
}

/**
* Post-copy hook.
* <p>
* This method is called after the original record is copied. It modifies the cloned record by setting a new name and nullifying the module.
* The cloned record is then saved, flushed, and refreshed in the database.
*
* @param originalRecord
* The original record that was copied.
* @param newRecord
* The newly created cloned record.
* @return The modified cloned record.
*/
@Override
public BaseOBObject postCopy(BaseOBObject originalRecord, BaseOBObject newRecord) {
CopilotFile originalFile = (CopilotFile) originalRecord;
CopilotFile cloneFile = (CopilotFile) newRecord;
cloneFile.setName("Copy of " + originalFile.getName());
cloneFile.setModule(null);

OBDal.getInstance().save(cloneFile);
OBDal.getInstance().flush();
OBDal.getInstance().refresh(cloneFile);
return cloneFile;
}
}
19 changes: 4 additions & 15 deletions src/com/etendoerp/copilot/process/CheckHostsButton.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

GPT Review for CheckHostsButton.java

Review

  • Estimated effort to review [1-5]:
    2, because the changes are straightforward, involving a method replacement and some imports. The complexity is low, but it requires understanding the context of the token generation.
  • Score: 85

Code feedback

  • File:
    src/com/etendoerp/copilot/process/CheckHostsButton.java
  • Language:
    java
  • Suggestion:
    Consider adding error handling or logging within the CopilotUtils.generateEtendoToken() method to ensure that any issues during token generation are captured and can be debugged easily. This will help in maintaining robustness and traceability in case of failures. [important]
  • Label:
    error handling
  • Existing code:
String token = CopilotUtils.generateEtendoToken();
if (token == null) {
    log4j.error("Token is null. Unable to proceed with host checks.");
    throw new OBException("Error when generating token.");
}
  • Improved code:
String token = CopilotUtils.generateEtendoToken();
if (token == null) {
    log4j.error("Token is null. Unable to proceed with host checks.");
    throw new OBException("Error when generating token.");
} else {
    log4j.info("Token generated successfully.");
}

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.openbravo.erpCommon.utility.OBMessageUtils;
import org.openbravo.model.ad.access.Role;
import org.openbravo.model.ad.access.User;
import org.openbravo.model.common.enterprise.Organization;
import org.openbravo.model.common.enterprise.Warehouse;

import com.etendoerp.copilot.util.CopilotUtils;
import com.smf.securewebservices.utils.SecureWebServicesUtils;
Expand Down Expand Up @@ -55,7 +57,7 @@ public class CheckHostsButton extends BaseProcessActionHandler {
protected JSONObject doExecute(Map<String, Object> parameters, String content) {
JSONObject checks;
try {
String token = getSecurityToken();
String token = CopilotUtils.generateEtendoToken();
if (token == null) {
log4j.error("Token is null. Unable to proceed with host checks.");
throw new OBException("Error when generating token.");
Expand All @@ -69,20 +71,7 @@ protected JSONObject doExecute(Map<String, Object> parameters, String content) {
return checks;
}

/**
* Generates a security token based on the current user's role and credentials.
*
* @return A string representing the generated token.
* @throws Exception
* If there is an error generating the token.
*/
private static String getSecurityToken() throws Exception {
OBContext context = OBContext.getOBContext();
Role role = OBDal.getInstance().get(Role.class, context.getRole().getId());
User user = OBDal.getInstance().get(User.class, context.getUser().getId());

return SecureWebServicesUtils.generateToken(user, role);
}

/**
* Checks the Etendo host connectivity and configuration.
Expand Down Expand Up @@ -234,4 +223,4 @@ private HttpURLConnection createConnection(String urlString, String token) throw
connection.setRequestProperty("Accept", CONTENT_TYPE);
return connection;
}
}
}
10 changes: 1 addition & 9 deletions src/com/etendoerp/copilot/util/CopilotUtils.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

GPT Review for CopilotUtils.java

Review

  • Estimated effort to review [1-5]:
    2, because the changes are straightforward, involving a refactoring of the generateEtendoToken method to use a different utility function. The complexity is low, but a careful check is needed to ensure the new method getEtendoSWSToken is correctly implemented and integrated.
  • Score: 85

Code feedback

  • File:
    src/com/etendoerp/copilot/util/CopilotUtils.java
  • Language:
    java
  • Suggestion:
    Ensure that the getEtendoSWSToken method handles null values appropriately for its parameters. If null is passed as a parameter, it could lead to unexpected behavior or exceptions if not handled properly. Consider adding null checks or default values within the getEtendoSWSToken method. [important]
  • Label:
    possible issue
  • Existing code:
return getEtendoSWSToken(OBContext.getOBContext(), null);
  • Improved code:
OBContext context = OBContext.getOBContext();
if (context == null) {
    throw new IllegalArgumentException("OBContext cannot be null");
}
return getEtendoSWSToken(context, null);

Original file line number Diff line number Diff line change
Expand Up @@ -631,15 +631,7 @@ public static String getAppSourceContent(CopilotAppSource appSource) throws IOEx
}

public static String generateEtendoToken() throws Exception {

OBContext context = OBContext.getOBContext();
Organization currentOrganization = OBDal.getInstance().get(Organization.class,
context.getCurrentOrganization().getId());
Role role = OBDal.getInstance().get(Role.class, context.getRole().getId());
User user = OBDal.getInstance().get(User.class, context.getUser().getId());
Warehouse warehouse = OBDal.getInstance().get(Warehouse.class, context.getWarehouse().getId());
return SecureWebServicesUtils.generateToken(user, role, currentOrganization,
warehouse);
return getEtendoSWSToken(OBContext.getOBContext(), null);
}

/**
Expand Down
Loading