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

ROAD-546: Add comments to stories #304

Merged
merged 25 commits into from
Oct 9, 2023
Merged

ROAD-546: Add comments to stories #304

merged 25 commits into from
Oct 9, 2023

Conversation

torresga
Copy link
Contributor

@torresga torresga commented Sep 11, 2023

Jira Ticket

Jira ticket associated with this PR

Motivation / Context

Context can be found in this issue: #195

Basically, this PR adds comments to Stories in Points. This is to help keep information together that usually ends up in other channels (such as Slack). The comments are also exportable. The comments don't end up in the action plan.

QA / Testing Instructions

Screenshots:

Screen Shot 2023-09-15 at 9 32 43 AM

I will abide by the code of conduct.

@torresga torresga marked this pull request as ready for review September 15, 2023 13:29
@torresga torresga changed the title Road-547: Add comments to stories ROAD-547: Add comments to stories Sep 15, 2023
@torresga torresga changed the title ROAD-547: Add comments to stories ROAD-546: Add comments to stories Sep 15, 2023
Copy link
Contributor

@arielj arielj left a comment

Choose a reason for hiding this comment

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

hey @torresga @hmdros, I added some comments related to the 3 stories

app/controllers/comments_controller.rb Outdated Show resolved Hide resolved
app/controllers/comments_controller.rb Outdated Show resolved Hide resolved
app/controllers/comments_controller.rb Outdated Show resolved Hide resolved
app/controllers/comments_controller.rb Outdated Show resolved Hide resolved
app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
spec/controllers/comments_controller_spec.rb Outdated Show resolved Hide resolved
spec/controllers/comments_controller_spec.rb Outdated Show resolved Hide resolved
app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
app/controllers/comments_controller.rb Show resolved Hide resolved
app/views/stories/show.html.erb Outdated Show resolved Hide resolved
@hmdros hmdros requested a review from arielj September 20, 2023 11:08
Copy link
Contributor

@arielj arielj left a comment

Choose a reason for hiding this comment

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

Added more comments, specially on the export that is currently broken

app/views/comments/_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment.html.erb Outdated Show resolved Hide resolved
app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
app/controllers/stories_controller.rb Show resolved Hide resolved
app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
@hmdros hmdros requested a review from arielj September 20, 2023 22:54
Copy link
Contributor

@arielj arielj left a comment

Choose a reason for hiding this comment

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

I left some comments, some are just suggestions, but the export with comments is still not correct

app/views/comments/_comment.html.erb Outdated Show resolved Hide resolved
app/views/comments/_form.html.erb Outdated Show resolved Hide resolved
app/controllers/stories_controller.rb Show resolved Hide resolved
@story = Story.find(params[:story_id])
rescue ActiveRecord::RecordNotFound
flash[:error] = "Project or Story not found"
redirect_to projects_path
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should handle the 2 possible exceptions individually, so in the cases where the project exists and the story doesn't we can redirect to the project instead of redirecting to the index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but should we do that in this ticket or create another issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, this is adding this code, we could create another story though, it's not a blocker so I'm fine ignoring it for now

Copy link
Contributor

@arielj arielj left a comment

Choose a reason for hiding this comment

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

I added 2 comments that are not blockers

I did find another error with the exports

app/views/comments/_comment.html.erb Outdated Show resolved Hide resolved
@story = Story.find(params[:story_id])
rescue ActiveRecord::RecordNotFound
flash[:error] = "Project or Story not found"
redirect_to projects_path
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, this is adding this code, we could create another story though, it's not a blocker so I'm fine ignoring it for now

app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
@hmdros hmdros requested a review from arielj September 25, 2023 14:35
@aisayo aisayo temporarily deployed to points-road-547-ai4ls7ewh3p73r September 26, 2023 00:28 Inactive
Copy link
Member

@mateusdeap mateusdeap left a comment

Choose a reason for hiding this comment

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

I think we need to make sure that importing the generated CSV works. As per Atlassian's docs, I believe that the comments will need to be quoted.

Then again, I'm assuming this has worked so far, so maybe nothing else is needed? Their documentation isn't super detailed as it should be, honestly. They mention how to format a comment and say that we can just put multiple comments in separate columns, but don't specify if each comment needs it's own header or if it will just assume that subsequent values are comments.

I believe we need @ABizzinotto to QA this last bit of the feature.

I'm approving the PR, but I believe we need to address the above before merging. Unless someone has tried it already

@arielj
Copy link
Contributor

arielj commented Sep 26, 2023

I think we need to make sure that importing the generated CSV works. As per Atlassian's docs, I believe that the comments will need to be quoted.

Then again, I'm assuming this has worked so far, so maybe nothing else is needed? Their documentation isn't super detailed as it should be, honestly. They mention how to format a comment and say that we can just put multiple comments in separate columns, but don't specify if each comment needs it's own header or if it will just assume that subsequent values are comments.

I believe we need @ABizzinotto to QA this last bit of the feature.

I'm approving the PR, but I believe we need to address the above before merging. Unless someone has tried it already

The CSV export compatible with JIRA is in a different story https://ombulabs.atlassian.net/browse/VLT-5 that was not worked on yet, that is not part of this PR

@aisayo aisayo temporarily deployed to points-road-547-kez5arctpflqij September 27, 2023 01:51 Inactive
@ABizzinotto ABizzinotto added Ready to Merge Indicates the PR has passed manual testing and is ready to be merged. and removed Ready to Merge Indicates the PR has passed manual testing and is ready to be merged. labels Sep 27, 2023
@etagwerker etagwerker temporarily deployed to points-road-547-kez5arctpflqij September 27, 2023 21:36 Inactive
@arielj arielj merged commit 3c7786f into main Oct 9, 2023
2 checks passed
@arielj arielj deleted the ROAD-547 branch October 9, 2023 13:57
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.

7 participants