Skip to content
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

Add timestamp and biddercode to event URL - refactored #653

Merged
merged 3 commits into from
Apr 6, 2020

Conversation

rpanchyk
Copy link
Contributor

@rpanchyk rpanchyk commented Apr 3, 2020

See parent PR #635

@rpanchyk rpanchyk requested a review from DGarbar April 3, 2020 18:21
added timestamp field to PutObject
Comment on lines +85 to +87
if (timestamp != null) {
try {
Long.parseLong(timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NumberUtils.isDigits() or StringUtils.isDigits(), (but we still need to check for null bc this value is not required)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both util methods doesn't guaranteed Long range value checking, so NumberFormatException can be thrown. Don't think they are useful here.

Comment on lines +199 to +200

updatedPutObjects.add(builder.build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest

   for (PutObject putObject : putObjects) {

            JsonNode value = putObject.getValue();
            if (biddersAllowingVastUpdate.contains(putObject.getBidder()) && value != null) {
                final String updatedVastXml = modifyVastXml(value.asText(), putObject.getBidid(),
                        putObject.getBidder(), accountId, putObject.getTimestamp());
                value = new TextNode(updatedVastXml);
            }

            updatedPutObjects.add(putObject.toBuilder()
                    // remove "/vtrack" specific fields
                    .bidid(null)
                    .bidder(null)
                    .timestamp(null)
                    
                    .value(value)
                    .build());
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of taste-)

@@ -1072,35 +1071,63 @@ public void cachePutObjectsShouldModifyVastAndCachePutObjects() throws IOExcepti
// given
final PutObject firstPutObject = PutObject.builder()
.type("xml")
.bidid("biddid1")
.bidid("bidId1")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also added .timestamp(1231221L) to test

@rpanchyk rpanchyk merged commit 37f20c2 into add-timestamp-to-ivent Apr 6, 2020
@rpanchyk rpanchyk deleted the add-timestamp-to-ivent-refactored branch April 6, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants