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

TDL 17738 sync conversations stream correctly #54

Open
wants to merge 8 commits into
base: TDL-20356-update-function-based-to-class-based
Choose a base branch
from

Conversation

prijendev
Copy link

@prijendev prijendev commented Sep 6, 2022

Description of change

  • Updated logic for conversation stream to overwrite updated_at value by last_edited_at field if it is available.
  • If last_edited_at is not None then assign it's value to updated_at field.
  • Added unit tests for the changes.

Manual QA steps

  • For conversation record with last_edited_at field value, check updated_at is updated.
  • For conversation record with last_edited_at field null value, check updated_at is not updated.

Risks

Rollback steps

  • revert this branch

@prijendev prijendev changed the base branch from master to TDL-20356-update-function-based-to-class-based September 6, 2022 09:43
Overwrite updated__at value.
"""
if record.get("last_edited_at"):
record["updated_at"] = max(record["updated_at"], record["last_edited_at"])

Choose a reason for hiding this comment

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

Please add comments here regarding why the above changes are needed, and we can't use updated_at directly.

Copy link
Author

Choose a reason for hiding this comment

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

Added code comments to explain above if condition.

"tickets": {"updated_at": "2020-03-16T00:00:00Z"},
}

@parameterized.expand([

Choose a reason for hiding this comment

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

Add comments like #["test_name", "selected_streams"......] to explain each arguments.

Copy link
Author

Choose a reason for hiding this comment

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

Added comments to explain arguments.

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.

3 participants