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

Close/Open Comments #159

Closed
gabelula opened this issue Dec 10, 2016 · 4 comments
Closed

Close/Open Comments #159

gabelula opened this issue Dec 10, 2016 · 4 comments
Assignees
Labels

Comments

@gabelula
Copy link

Story: https://www.pivotaltracker.com/n/projects/1863625/stories/134297345

Expected behavior

  • The 'Close Stream' button in the 'Configure Stream' should have a box to write down the close message. (Otherwise we should have the Asset.closedMessage field not in the Asset but in Settings).
  • The endpoint for changing the status of an Asset should change to use closedAt, closedMessage instead of status field

Actual behavior

  • Closing and opening comments is not working (backend and frontend not sync)

Steps to reproduce behavior

  • open the comment thread
  • reload the page
  • check if it is still open
@gabelula gabelula added the bug label Dec 10, 2016
@wyattjoh
Copy link
Collaborator

I added a comment on a related PR: #161 (comment)

I just don't think that an asset holds the same responsibilities that a comment has, so I don't think that we necessarily need to break out the functionality from the closedAt field already provides

The status of a asset is based on a temporal value (the time) and not the "most recent status" so the determination of the current status becomes much more complex on the FE and the BE

If this were just a text field of status (like in the comment) I'd agree that it would work. But the closed status doesn't make sense that we "need" to store the history especially because of the overhead of managing the data models for it.

I just don't think this is the right approach.

@gabelula
Copy link
Author

How do you open back the asset with the closedAt approach? Just null back the field? I'm ok with that for now actually. We just needed to fix the backend to have it working with the frontend. I can modify the PR for that.

@jde any thoughts about this?

@wyattjoh
Copy link
Collaborator

wyattjoh commented Dec 12, 2016

closedAt: null or closedAt: false would do the same thing there I think @gabelula

@gabelula
Copy link
Author

It get resolved in the PR #161 ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants