SED-4417-grid-layout-for-executions-report#118
Conversation
Summary of ChangesHello, 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 introduces Highlights
Changelog
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces user ID tracking for object creation and modification in AbstractTrackedObject by adding creationUserId and lastModificationUserId fields of type ObjectId. The changes are straightforward, but there are a couple of points for consideration. Firstly, the use of org.bson.types.ObjectId may impact database portability, particularly for PostgreSQL, and needs careful handling. Secondly, the changes are not accompanied by tests to verify the persistence of the new fields. I've added comments with more details on these points. Also, the PR title 'SED-4417-grid-layout-for-executions-report' does not seem to match the content of the changes, which might cause confusion.
|
|
||
| private Date creationDate; | ||
| private String creationUser; | ||
| private ObjectId creationUserId; |
There was a problem hiding this comment.
The new field creationUserId (and lastModificationUserId on line 38) uses org.bson.types.ObjectId. This introduces a dependency on the BSON library, which is specific to MongoDB. Given that the project also supports PostgreSQL (as suggested by the step-framework-collections-postgresql module), this could cause issues with persistence on non-MongoDB databases. Please ensure that there is a clear strategy for handling ObjectId types in PostgreSQL, for example, by mapping them to a VARCHAR or UUID type. If this model is intended to be database-agnostic, using a more standard type like java.util.UUID or String for IDs might be a more portable solution.
| public ObjectId getCreationUserId() { | ||
| return creationUserId; | ||
| } | ||
|
|
||
| public void setCreationUserId(ObjectId creationUserId) { | ||
| this.creationUserId = creationUserId; | ||
| } | ||
|
|
||
| public ObjectId getLastModificationUserId() { | ||
| return lastModificationUserId; | ||
| } | ||
|
|
||
| public void setLastModificationUserId(ObjectId lastModificationUserId) { | ||
| this.lastModificationUserId = lastModificationUserId; | ||
| } |
There was a problem hiding this comment.
While these getters and setters are correctly implemented, the pull request does not seem to include any new or updated tests to verify that the creationUserId and lastModificationUserId fields are correctly persisted and retrieved. Adding test coverage for new functionality is crucial for maintaining code quality and preventing future regressions. Please consider adding tests that cover these new fields.
|
|
||
| private Date creationDate; | ||
| private String creationUser; | ||
| private ObjectId creationUserId; |
There was a problem hiding this comment.
@david-stephan Not sure about the introduction of these 2 new fields. Many places in Step refer to the username as the key for the user and I don't think we support its change
There was a problem hiding this comment.
@jeromecomte Step does support to edit and rename users in the users' admin settings.. This was my comment on the JIRA ticket as reference: "I added the user ID to the tracked object metadata because it is safer to use to verify if the authenticated user is the owner of the layout (otherwise if the user is renamed, the logic wouldn’t work anymore). We have other entities that extends Tracked objects (parameters, AP, resources, alerting rules) but only as information, so I did not touch them (the user IDs will remain null for them)."
There was a problem hiding this comment.
As discussed, let's use the username as key and implement hooks to change the properties referencing usernames upon username change
2 participants
No description provided.