git-bug
git-bug copied to clipboard
MichaelMure
Editing comments (in TermUI and WebUI) has no effect
The error seem to be introduced with this commit: https://github.com/MichaelMure/git-bug/commit/24155156223595e48f215e87cf9ce7895e22f4a7
Reverting this line back to commentId = op.target seems to fix the issue. But I'm not sure if this is the right way to solve the problem. This discussion seems to be related, maybe reverting the line was just forgotten?
https://github.com/MichaelMure/git-bug/commit/24155156223595e48f215e87cf9ce7895e22f4a7 was correct. The problem is that there is a conversion missing.
EditCommentOperation correctly use an Operation's Id as the target. The problem is that the termui (and webUI I suppose) is using a comment's ID (a combined ID) directly without conversion. Where exactly this conversion should happen, I'm not so sure.
Also, having a different type (entity.CombinedId) might make sense to avoid those class of bugs. We would know what kind of Id we have and the compiler could enforce the correct usage.
Tough problem. I added a CombinedId type and replaced the comment.id type (entity.Id) with CombinedId. I also adjusted the function signatures accordingly. But one Problem persists. The TimeLineItem interface requires an Id, but the CommentTimeLineItem uses a CombinedId, as it should match the Comment.Id (of type CombinedId).
...is using a comment's ID (a combined ID) directly without conversion.
I don't know how they they should be converted. :grimacing:
Also, having a different type (entity.CombinedId) might make sense to avoid those class of bugs. We would know what kind of Id we have and the compiler could enforce the correct usage.
If that works without introducing other complications, then that would help a lot, I guess.
If it were up to me I would aim for separate types for different kinds of Ids. But in case that does not work out as expected, one can add a field plainId to the Comment struct to store both Ids for each Comment. Then one has always the correct Id at his disposal.
FYI I started working on that in https://github.com/MichaelMure/git-bug/pull/664 by having a dedicated CombinedId type and use that in various places, but I'm yet to figure out the clean way to join in the middle. Should be doable though.
@MichaelMure I fiddled around with your PR on my own fork. I think I found the Problem with the WebUI. The WebUI passes the CommentId (the TimelineId of type CombinedId) to the Target of the EditCommentMutation. This Id was then passed wrongly around. Instead the Resolver has to extract the OperationId of the CommentId and pass this OpId around. I fixed this and now the WebUI fails with the same error as the TermUI.
One problem left, the extracted OpId is only a Prefix and will fail the id validation due to "invalid length"... :'( (Possible relevant note by Michael was given here: https://github.com/GlancingMind/git-bug/pull/57#issuecomment-852936828)
Got it! Editing of comments via CLI, TermUI and WebUI work again. ~https://github.com/GlancingMind/git-bug/pull/57~ https://github.com/GlancingMind/git-bug/pull/61 Unfortunately the fix looks rather bad... duplicating the same code in ~three~ four different places screams for a better solution. I could remove the redundant code by placing it into EditCommentOperation::Apply, but then the EditCommentOperationTarget must be a CombinedId. Making the Target a TimelineItem rather than an Operation.
Side Note: Is the separation of a TimelineItem and an Operation needed or could an Operation not be a TimelineItem?
https://github.com/MichaelMure/git-bug/pull/835 brought to light the bug that likely cause this. Somewhere in the code an EditCommentOperation was created, the Id() was taken to register the change in the timeline, then metadata were added to that operation, changing the data and the Id. A newly created assertion failed in that branch, fix pending.