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 option to sentAt field to be null #345

Merged
merged 6 commits into from
May 5, 2022

Conversation

edsonjab
Copy link
Contributor

Override method create on Batch class to lend to user define his own date on sentAt field or use a null value.

Issue #340

@pooyaj
Copy link
Contributor

pooyaj commented May 2, 2022

@edsonjab please check out the comment on the issue: #340

Copy link

@wenxi-zeng wenxi-zeng left a comment

Choose a reason for hiding this comment

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

the changes does not address the issue, since Batch.create() is only used internally to create a batch.

you'll need to do the following:

  1. in Message add a new nullable field sentAt
  2. in MessageBuilder add a new field sentAt and make changes accordingly. you can take userId as an example for what needs to be done
  3. in MessageBuilder.realBuild add a new parameter sentAt and make changes accordingly.
  4. discard all your changes in Batch. in create method do the following
if any message in batch has `sentAt` not null
    use that value to create batch
otherwise
    use `new Date()` to create batch

now the users can use builder.sentAt() to modify the value

@edsonjab edsonjab requested a review from wenxi-zeng May 5, 2022 19:36
@nd4p90x nd4p90x requested a review from pooyaj May 5, 2022 21:13
analytics-core/.factorypath Outdated Show resolved Hide resolved
}

public abstract List<Message> batch();

@Nullable

Choose a reason for hiding this comment

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

no need to be nullable here

@@ -26,6 +28,22 @@ public void create(TestUtils.MessageBuilderFactory factory) {
assertThat(batch.context()).isEqualTo(context);
}

@Test
public void createWithSentAt(TestUtils.MessageBuilderFactory factory) throws ParseException {

Choose a reason for hiding this comment

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

better to have another test with messages has sentAt as nulls, so we have both paths covered.

Copy link

@wenxi-zeng wenxi-zeng left a comment

Choose a reason for hiding this comment

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

rest LGTM

@pooyaj pooyaj merged commit b578343 into segmentio:master May 5, 2022
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.

Including sentAt in the Batch body of the HTTP request forces the use of a segment server computed timestamp
3 participants