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

Fix notification request specs #1450

Merged
merged 9 commits into from
Jan 14, 2019

Conversation

Zhao-Andy
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor

Description

Fixes all the notification request specs that were xited. This should help us feel more comfortable making changes to notifications.

Also made a small (performance?) improvement by using an already defined variable.

Added to documentation?

  • no documentation needed

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 2, 2019
Copy link
Contributor Author

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Two tests are still xited. These two specs check how follow and reaction notifications aggregate. We changed it since the test was written so I left it as-is for now.

@@ -21,5 +21,5 @@
</span>
</div>
<% else %>
<%= render "aggregated_reactions", notification: notification %>
<%= render "aggregated_reactions", notification: notification, siblings: siblings %>
Copy link
Contributor Author

@Zhao-Andy Zhao-Andy Jan 2, 2019

Choose a reason for hiding this comment

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

Passing along siblings from this file to aggregated_reactions instead of defining it twice (once in this file, another in aggregated_reactions).

expect(response.body).to include message
end

xit "renders the proper message for a single private reaction" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer have "private user" reactions.

xit "renders the comment's processed HTML" do
expect(response.body).to include CGI.escapeHTML(comment.processed_html)
it "renders the comment's processed HTML" do
expect(response.body).to include comment.processed_html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't need to escape HTML that is already safe.

@Zhao-Andy Zhao-Andy self-assigned this Jan 11, 2019
@Zhao-Andy Zhao-Andy changed the title Specs/fix notification specs Fix notification requests specs Jan 11, 2019
@Zhao-Andy Zhao-Andy changed the title Fix notification requests specs Fix notification request specs Jan 11, 2019
Copy link
Contributor

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

LGTM

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 14, 2019
@maestromac maestromac merged commit 7726acf into forem:master Jan 14, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants