-
Notifications
You must be signed in to change notification settings - Fork 26
(breaking change): Remove API call to query for a user's most recent notification. Code locations by Detect will be created without querying notifications API. #475
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
Changes from 2 commits
0e1a8fa
d8a20ad
84dfbfd
7e10c6a
b0411c3
326f1a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,7 @@ public Date getLatestNotificationDate() throws IntegrationException { | |
| * @throws IntegrationException | ||
| */ | ||
| public Date getLatestUserNotificationDate(UserView userView) throws IntegrationException { | ||
| logger.warn("Fetching latest notification date for user via a low‑performance API."); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are warning this method is low performance which API is more performant? Is the more performant API used and this method is a fallback? By warning that this API is in use if we don't use the more performant API and this message appears in the logs a customer may question why are we using an API with low performance and why aren't we using a more performant API. So we will need to be prepared for those questions if we are now informing on the performance of this API.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is a very good point you raise, and maybe the removal of this method entirely is a better move. Since this is a shared library, I kept the changes limited to what affects Detect only. If I can confirm that Alert also does not use this method, we can safely get rid of it. Below are answers to your questions that will hopefully clear things up:
Sorry for the wall of text, hope that answers your questions. Do you suggest the complete removal of this method? (affects upstream methods and mostly just test cases within this lib) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. Your points are valid here I just wanted to avoid confusion for our customers and support. If you remove the method that would be a breaking change, so the version needs to change to the next major version. I have looked and Alert does not make this call so it will not be affected either.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the method getLatestuserNotification() and replaced any uses with the current date instead (this is what Detect does now as well). The reason for fetching latest notification timestamp was just to create a time window for checking new notifications, so the current timestamp works for our use cases just as well and all tests pass. +Bumped major version. @psantos1113 @DanaMaxfield Would you mind taking a look when you get a chance at the last 2 commits? Thank you! |
||
| BlackDuckRequestBuilder blackDuckRequestBuilder = createLatestDateRequestBuilder(); | ||
| BlackDuckMultipleRequest<NotificationUserView> requestMultiple = blackDuckRequestBuilder.buildBlackDuckRequest(userNotificationsResponses.apply(userView)); | ||
| List<NotificationUserView> userNotifications = blackDuckApiClient.getSomeResponses(requestMultiple, 1); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.