-
Notifications
You must be signed in to change notification settings - Fork 3.7k
LPD-69423 highlight sidebar menu correctly #6698
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
base: master
Are you sure you want to change the base?
Conversation
…InfoItemUtil#getGroupId
… ViewSpacesSectionDisplayContext
Summary of ChangesHello @robertoDiaz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the sidebar menu was not correctly highlighting the active item. It introduces a new test case to validate the highlighting logic for asset libraries and refines utility methods to ensure accurate retrieval of group IDs and depot entries from various object types, including Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request involves changes to the ViewSpacesSectionDisplayContextTest.java and InfoItemUtil.java files. The changes include adding new imports, a new test method testGetProps, and modifications to the getDepotEntryId and getGroupId methods in InfoItemUtil.java. The new test method tests the retrieval of properties, specifically focusing on asset libraries and their active status. The modifications in InfoItemUtil.java enhance the logic for retrieving DepotEntry and GroupId based on the request object.
| if (depotEntry == null) { | ||
| depotEntry = DepotEntryLocalServiceUtil.fetchGroupDepotEntry( | ||
| getGroupId(httpServletRequest)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetchGroupDepotEntry method is used without proper error handling. If fetchGroupDepotEntry returns null, the code will continue without a valid depotEntry, potentially leading to a NullPointerException later on. It's crucial to handle the case where no DepotEntry is found for the group.
Consider adding a check for null after calling fetchGroupDepotEntry and either logging an error or throwing an exception to prevent further execution with invalid data.
if (depotEntry == null) {
depotEntry = DepotEntryLocalServiceUtil.fetchGroupDepotEntry(
getGroupId(httpServletRequest));
if (depotEntry == null) {
// Log an error or throw an exception
// Example:
// throw new Exception("No DepotEntry found for group " + getGroupId(httpServletRequest));
return 0; // Or throw an exception
}
}| @Test | ||
| public void testGetProps() throws Exception { | ||
| AssetLibrary assetLibrary1 = _addAssetLibrary(); | ||
|
|
||
| _addAssetLibrary(); | ||
|
|
||
| DepotEntry depotEntry = _depotEntryLocalService.getDepotEntry( | ||
| assetLibrary1.getId()); | ||
|
|
||
| ObjectEntryFolder objectEntryFolder = | ||
| _objectEntryFolderLocalService.addObjectEntryFolder( | ||
| null, depotEntry.getGroupId(), TestPropsValues.getUserId(), | ||
| ObjectEntryFolderConstants. | ||
| PARENT_OBJECT_ENTRY_FOLDER_ID_DEFAULT, | ||
| RandomTestUtil.randomString(), | ||
| HashMapBuilder.put( | ||
| LocaleUtil.getDefault(), StringUtil.randomString() | ||
| ).build(), | ||
| StringUtil.randomString(), | ||
| ServiceContextTestUtil.getServiceContext()); | ||
|
|
||
| Map<String, Object> props = ReflectionTestUtil.invoke( | ||
| _getViewSpacesDisplayContext( | ||
| getMockHttpServletRequest(objectEntryFolder)), | ||
| "getProps", new Class<?>[0]); | ||
|
|
||
| Assert.assertNotNull(props); | ||
|
|
||
| JSONArray assetLibrariesJSONArray = (JSONArray)props.get( | ||
| "assetLibraries"); | ||
|
|
||
| for (int i = 0; i < assetLibrariesJSONArray.length(); i++) { | ||
| JSONObject assetLibraryJSONObject = | ||
| assetLibrariesJSONArray.getJSONObject(i); | ||
|
|
||
| if (Objects.equals( | ||
| assetLibraryJSONObject.get("id"), assetLibrary1.getId())) { | ||
|
|
||
| Assert.assertTrue(assetLibraryJSONObject.getBoolean("active")); | ||
| } | ||
| else { | ||
| Assert.assertFalse(assetLibraryJSONObject.getBoolean("active")); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test method testGetProps lacks proper cleanup after execution. Specifically, the ObjectEntryFolder created within the test is not deleted, which can lead to data pollution and potential interference with other tests. This can cause intermittent test failures and make the test suite unreliable.
Consider adding code to delete the created ObjectEntryFolder in a finally block or using a @After annotated method to ensure cleanup even if the test fails.
@Test
public void testGetProps() throws Exception {
AssetLibrary assetLibrary1 = _addAssetLibrary();
_addAssetLibrary();
DepotEntry depotEntry = _depotEntryLocalService.getDepotEntry(
assetLibrary1.getId());
ObjectEntryFolder objectEntryFolder = null;
try {
objectEntryFolder =
_objectEntryFolderLocalService.addObjectEntryFolder(
null, depotEntry.getGroupId(), TestPropsValues.getUserId(),
ObjectEntryFolderConstants.
PARENT_OBJECT_ENTRY_FOLDER_ID_DEFAULT,
RandomTestUtil.randomString(),
HashMapBuilder.put(
LocaleUtil.getDefault(), StringUtil.randomString()
).build(),
StringUtil.randomString(),
ServiceContextTestUtil.getServiceContext());
Map<String, Object> props = ReflectionTestUtil.invoke(
_getViewSpacesDisplayContext(
getMockHttpServletRequest(objectEntryFolder)),
"getProps", new Class<?>[0]);
Assert.assertNotNull(props);
JSONArray assetLibrariesJSONArray = (JSONArray)props.get(
"assetLibraries");
for (int i = 0; i < assetLibrariesJSONArray.length(); i++) {
JSONObject assetLibraryJSONObject =
assetLibrariesJSONArray.getJSONObject(i);
if (Objects.equals(
assetLibraryJSONObject.get("id"), assetLibrary1.getId())) {
Assert.assertTrue(assetLibraryJSONObject.getBoolean("active"));
}
else {
Assert.assertFalse(assetLibraryJSONObject.getBoolean("active"));
}
}
} finally {
if (objectEntryFolder != null) {
_objectEntryFolderLocalService.deleteObjectEntryFolder(objectEntryFolder);
}
}
}
No description provided.