Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upVarious small notifications fixes #174
Conversation
| parseInt(s3Helper.SQS_RETENTION, 10) * 1000) { | ||
| let startAtToCheck = startAt | ||
| let currentTime = new Date().getTime() | ||
| if (startAtToCheck && currentTime.toString().length - startAtToCheck.toString().length >= 3) { |
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 24, 2017
Contributor
this is really strange, are we correcting for improper user input?
if so, i would prefer we threw an error rather than silently correcting the error and multiplying by 1000.
This comment has been minimized.
This comment has been minimized.
SergeyZhukovsky
Oct 24, 2017
Author
Member
well, we accept startAt in seconds or milliseconds, I don't know for what reason it was done initially so, Android version sends in milliseconds, laptop does in seconds, not sure about iOS, I gues in milliseconds as well.
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 24, 2017
Contributor
hmm probably we should normalize the usage across platforms.
i looked again, and see the records are stored with Date.now() which returns milliseconds.
s3 listObjectsV2 startAfter lists things alphabetically in order, so we do support both milliseconds and seconds.
Can we add a new helper function normalizeTimestampToMs() which does the string length check and multiplies if needed?
| if (startAtToCheck && currentTime.toString().length - startAtToCheck.toString().length >= 3) { | ||
| startAtToCheck *= 1000 | ||
| } | ||
| if (!startAtToCheck || |
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 24, 2017
Contributor
minor: would prefer to extract this logic into a helper function shouldListObjects()
| @@ -211,11 +218,19 @@ RequestUtil.prototype.list = function (category, startAt, maxRecords, platform) | |||
| } | |||
|
|
|||
| return s3Helper.listNotifications(this.sqs, notificationParams, category, | |||
| `${this.apiVersion}/${encodeURIComponent(this.userId)}/${category}`, platform) | |||
| `${this.apiVersion}/${encodeURIComponent(this.userId).replace('%2F', '/')}/${category}`) | |||
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 24, 2017
Contributor
just for my info, what was going on with %2F here?
and Minor: since we reuse it below, maybe we can make a helper userIdEncoded() which returns encodeURIComponent(this.userId).replace('%2F', '/')
This comment has been minimized.
This comment has been minimized.
SergeyZhukovsky
Oct 24, 2017
Author
Member
it turned out that userId could contain /, which is several folders on s3 bucket in that case, we had to keep it / for notifications filter but replace symbols like =.
| @@ -108,6 +108,7 @@ RequestUtil.prototype.saveAWSCredentials = function (parsedResponse) { | |||
|
|
|||
| this.sqs = parsedResponse.sqs | |||
| this.sns = parsedResponse.sns | |||
| this.initialSyncDone = true | |||
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 24, 2017
Contributor
minor: it would be more semantic if this variable were named listInProgress because we don't set the var until we call listObjects().
also, initializing initialSyncDone = true doesn't make sense in the case we didn't do a sync yet, as we quickly set it to false. in case we keep initialSyncDone, maybe we should init as undefined, then check within listObjects whether initialSyncDone === false (rather than falsey).
|
just the last bit regarding inverted values between |
| return !startAtToCheck || | ||
| (currentTime - startAtToCheck) > parseInt(s3Helper.SQS_RETENTION, 10) * 1000 || | ||
| category !== proto.categories.BOOKMARKS || | ||
| this.listInProgress === false |
This comment has been minimized.
This comment has been minimized.
| * Sets if the initial sync done or not | ||
| * @param {boolean} | ||
| */ | ||
| RequestUtil.prototype.setInitialSyncDone = function (initialSyncDone) { |
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 24, 2017
Contributor
should we change this to setListInProgress and then use the inverse of initialSyncDone values?
|
|
SergeyZhukovsky commentedOct 24, 2017