-
Notifications
You must be signed in to change notification settings - Fork 0
adds monitoring to complaint detail timeline #218
Conversation
This PR is in draft because @lauranash18f is going to look it over for design, but the bulk of the code seems unlikely to change (at least, I'm hoping) if people are looking for something to review |
911927b
to
224fe15
Compare
@lauranash18f requests that outcome be displayed whenever the status is complete, so this is what we end up with CONCERN 😟 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the error raise is probably being punted to the error task, but I think an inline-comment would help make sure that implementation detail of raising the error that matches what the timeline event is expecting isn't forgotten
<div> | ||
<ul class="usa-list usa-list--unstyled"> | ||
<li><strong>Status:</strong> <%= event.status %></li> | ||
<% if event.status == "Complete" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion (non-blocking)] instead of sprinkling checks like this around I think it would be better to have an event.complete?
method (that likely delegates to the review object)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea!
def review_link | ||
# using content_tag for the link will properly escape any dangerous characters that sneak into name or monitoring url | ||
content_tag(:a, name, class: "usa-link", href: review.itams_url, target: "_blank") | ||
rescue Api::Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] where does this get raised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I had copied that over from the TTA activity report without thinking about how I pulled out the get_api_data_field
error for monitoring. If you think just commenting it for now is the way to go, I can do that and I'll add a note on the error issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's fine as long as the person implementing the error task is super aware of this being here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Description of change
Displays linked monitoring reviews in an individual complaint's timeline alongside TTA activity and complaint updates. I didn't see a design specifically for this so I just picked some fields that seemed like they would be nice to display. If we would prefer to take something like "status" and change it into verbage the way that the complaint updates look (in progress / etc) we could do that.
Acceptance Criteria
How to test
Fire up the complaint tracker, head to a complaint's page, add a RAN ID, and it should appear in the timeline.
Issue(s)
Checklist
To be completed by the submitter:
To the Reviewer
This project is using Conventional Comments to give structure
and context to PR comments. Please use these labels in your comments.