-
Notifications
You must be signed in to change notification settings - Fork 70
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
Closes #762 and #763 - Last login time of an user added #772
Conversation
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.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fylip97)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/users/AccountController.java, line 30 at r2 (raw file):
import rocks.inspectit.ocelot.user.UserService; import java.sql.Timestamp;
Please format the code, so that unused import are also be gone.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/users/AccountController.java, line 66 at r2 (raw file):
@GetMapping("account/token") public String acuireNewAccessToken(Authentication auth) { String timeStamp = new SimpleDateFormat("dd/MM/yyyy HH:mm").format(new Date());
Storing the time as a string can make it quite hard if you do some processing with it. E.g. if you want only users which have een logged in in the last 24h. Or if you want to show the time in a different format.
A better approach would be storing the current timestamp System.currentTimeMillis
. Then you can just use a table column of type BIGINT
or TIMESTAMP
in the database.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/users/AccountController.java, line 67 at r2 (raw file):
userService.getUserByName(auth.getName())
Please check that the result is not empty: .isPresent()
Basically it should not happen, because this endpoint requires a user, but better be safe.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/user/User.java, line 59 at r2 (raw file):
private boolean isLdapUser; @Column
Please add a short documentation.
/**
* Timestamp when the user ....
*/
components/inspectit-ocelot-configurationserver/src/main/resources/db/migration/V1.1__database__lastLoginAdded.sql, line 1 at r2 (raw file):
ALTER TABLE user
Imo, this modification leads to a new version of the schema. So the file name could be starting V2_
instead of V1.1_
?
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.
Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @mariusoe)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/users/AccountController.java, line 30 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please format the code, so that unused import are also be gone.
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/users/AccountController.java, line 66 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Storing the time as a string can make it quite hard if you do some processing with it. E.g. if you want only users which have een logged in in the last 24h. Or if you want to show the time in a different format.
A better approach would be storing the current timestamp
System.currentTimeMillis
. Then you can just use a table column of typeBIGINT
orTIMESTAMP
in the database.
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/users/AccountController.java, line 67 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
userService.getUserByName(auth.getName())
Please check that the result is not empty:
.isPresent()
Basically it should not happen, because this endpoint requires a user, but better be safe.
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/user/User.java, line 59 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please add a short documentation.
/** * Timestamp when the user .... */
Done.
components/inspectit-ocelot-configurationserver/src/main/resources/db/migration/V1.1__database__lastLoginAdded.sql, line 1 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo, this modification leads to a new version of the schema. So the file name could be starting
V2_
instead ofV1.1_
?
Done.
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.
Reviewed 4 of 4 files at r3, 1 of 1 files at r4, 4 of 4 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved
This change is