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

First draft to fix a bug when patching timeline events with extreme timestamp #1285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

longd3
Copy link
Contributor

@longd3 longd3 commented Oct 27, 2015

  • @spencerv discovered https://trello.com/c/GWSHI0ZW/32-provide-better-error-message-to-explain-that-you-can-t-update-the-wake-event-to-before-you-went-to-sleep , cc @jimmymlu
  • Bug Summary: In feedback feature, patching a timeline event with extreme timestamps (for example future ts) return bad timeline because of a failure to validate the patch request.
  • There are 2 validation processes. A> Feedback validations (to check if the input time make senses and doesn't break events order) and B> Timeline validations (to verify if the patched timeline meet sanity requirements)
  • This PR adds another validation to process B to enforce such that the patched timeline have total sleep duration GTE 120 minues (please advise if you think otherwise). But after discussing with @pims, we agree that more rigid checks need to be added to process A
  • @benejoseph: would you mind look at process A and advise the least intrusive way to handle that.

@longd3 longd3 changed the title First draft to a bug when patching timeline events timestamp First draft to fix a bug when patching timeline events timestamp Oct 27, 2015
@longd3 longd3 changed the title First draft to fix a bug when patching timeline events timestamp First draft to fix a bug when patching timeline events with extreme timestamp Oct 27, 2015
@pims
Copy link
Contributor

pims commented Oct 27, 2015

As discussed, this is only "fixing" by throwing an exception and not returning the timeline.
Let's make sure we don't accept invalid input to prevent this from happening.

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