Do not render report flag for reviews by the current user #6620

seanprashad · 2018-10-13T01:38:31Z

Fixes mozilla/addons#12433: "Flag" should be removed from the profile page

This PR prevents the "flag" button from being rendered for reviews made by the current user. Essentially, if Sean leaves a review, he should not be allowed to flag his own comment.

Before:

After:

codecov-io · 2018-10-13T23:15:04Z

Codecov Report

Merging #6620 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#6620      +/-   ##
==========================================
+ Coverage   97.84%   97.84%   +<.01%     
==========================================
  Files         240      241       +1     
  Lines        6549     6553       +4     
  Branches     1242     1243       +1     
==========================================
+ Hits         6408     6412       +4     
  Misses        126      126              
  Partials       15       15

Impacted Files	Coverage Δ
src/amo/components/AddonReviewCard/index.js	`100% <ø> (ø)`	⬆️
src/amo/pages/AddonReviewList/index.js	`100% <0%> (ø)`	⬆️
src/amo/reducers/versions.js	`100% <0%> (ø)`	⬆️
src/amo/components/AddonSummaryCard/index.js	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e7386e...54b1274. Read the comment docs.

seanprashad · 2018-10-13T23:16:08Z

tests/unit/amo/components/TestAddonReviewCard.js

@@ -562,11 +569,12 @@ describe(__filename, () => {
  });

  it('lets you flag a developer reply', () => {
-    const { reply } = _setReviewReply();
-    const root = renderReply({ reply });
+    const review = createReviewAndSignInAsUnrelatedUser();


This test was checking a developer's reply to an existing review. I think I changed the logic behind it - would appreciate feedback.

This test was checking a developer's reply to an existing review. I think I changed the logic behind it - would appreciate feedback.

Why did you need to change the test? I put it back to how it was and it passed for me. Maybe it was related to not being signed in?

bobsilverberg

This is a good start, thanks @seanprashad, but I think we only want to remove the "Flag" option on the User Profile screen, and not on every screen. I had a discussion with @kumar303 about this awhile ago, but I cannot find the issue where we discussed it.

He felt that we should keep the "Flag" menu, even for reviews by the same user, on the Review Listing screen, but it was fine to remove it from the User Profile screen.

@kumar303 Can you confirm that is what we should be doing for this issue?

kumar303 · 2018-10-16T16:38:47Z

I think the direction of this patch is good -- hiding the flag menu for all screens. I'd rather implement this feature consistently rather than just for the user profile page. I personally don't like hidden UI controls because it can be confusing to know what state you're in but everyone else argued against that so I concede 😁

kumar303

Thanks @seanprashad ! This mostly looks good. I have one change request.

kumar303 · 2018-10-16T16:51:25Z

src/amo/components/AddonReviewCard/index.js

@@ -481,7 +481,8 @@ export class AddonReviewCardBase extends React.Component<InternalProps> {
          </a>
        ) : null}

-        {flaggable && review ? (
+        {/* Do not render the "flag" button for replies made by the current user */}
+        {flaggable && review && siteUser && siteUser.id !== review.userId ? (


If the user is signed out, the Flag link should always be visible. Otherwise, if someone wants to flag a review, they might not know that they have to sign in first.

This is what it looks like (there is actually a style regression that I will fix right now! https://github.com/mozilla/addons-frontend/issues/6656)

This test is responsible for making sure that the "flag" button is hidden for replies made by the current user.

These tests are affected by the previous commit, which hides the "flag" button for reviews made by the current user.

seanprashad · 2018-10-17T19:51:25Z

@kumar303 I've updated the logic to always render the flag button if the user isn't logged in:

We should always render the "flag" button if a user is not authenticated because users should know that they have to sign in first before flagging a review.

kumar303

Thanks for the fix-ups. I have a few more change requests.

kumar303 · 2018-10-17T21:09:57Z

src/amo/components/AddonReviewCard/index.js

@@ -481,7 +486,7 @@ export class AddonReviewCardBase extends React.Component<InternalProps> {
          </a>
        ) : null}

-        {flaggable && review ? (
+        {flaggable && review && showFlagButton ? (


I liked how simple this was in your first patch. It was a lot easier to read. What about this?

Suggested change

{flaggable && review && showFlagButton ? (

{flaggable && review && (!siteUser || siteUser.id !== review.userId) ? (

kumar303 · 2018-10-17T21:13:57Z

src/amo/components/AddonReviewCard/index.js

@@ -481,7 +486,7 @@ export class AddonReviewCardBase extends React.Component<InternalProps> {
          </a>
        ) : null}

-        {flaggable && review ? (
+        {flaggable && review && showFlagButton ? (


You also need to add a test that makes sure the flag button is not hidden when the user is signed out. After writing the test, you can comment out the part that allowed non-signed in users to see the flag link; the test should fail. This helps you know that the test is working.

kumar303 · 2018-10-17T21:17:46Z

tests/unit/amo/components/TestAddonReviewCard.js

@@ -562,11 +569,12 @@ describe(__filename, () => {
  });

  it('lets you flag a developer reply', () => {
-    const { reply } = _setReviewReply();
-    const root = renderReply({ reply });
+    const review = createReviewAndSignInAsUnrelatedUser();


This test was checking a developer's reply to an existing review. I think I changed the logic behind it - would appreciate feedback.

Why did you need to change the test? I put it back to how it was and it passed for me. Maybe it was related to not being signed in?

kumar303 · 2018-10-17T21:18:49Z

tests/unit/amo/components/TestAddonReviewCard.js

@@ -547,7 +554,7 @@ describe(__filename, () => {
  });

  it('lets you flag a review', () => {
-    const review = _setReview(fakeReview);


I think you can put this back now, too. The test passes for me.

kumar303 · 2018-10-17T21:19:44Z

src/amo/components/AddonReviewCard/index.js

@@ -423,6 +423,11 @@ export class AddonReviewCardBase extends React.Component<InternalProps> {
      (review.userId === siteUser.id ||
        (this.isReply() && this.siteUserCanManageReplies()));

+    /* Do not render the "flag" button for reviews made by the current user */


Prefix comments with // (double slashes) when they don't need to span multiple lines.

These were initially changed as the new implementation hid the flag button for users who weren't logged in. We can now revert these as we display the flag button for all replies that do not belong to the current user, regardless if they are signed in or not.

seanprashad · 2018-10-18T02:32:33Z

Thanks for the great feedback @kumar303! ✌🏽I've reverted the other tests back and added a new test to check for the flag button when the user wasn't authenticated. I think this should be good to 🚢

Edit: Not sure if it's just me, but I can't reply to the third comment you made for your review - there's no textbox. I don't think I've seen this behaviour before:

kumar303 · 2018-10-18T14:13:04Z

Not sure if it's just me, but I can't reply to the third comment you made for your review

It's not just you. Github is buggy. I copied/pasted the quote of what I was replying to because I had actually pressed the reply button on your last comment and I knew Github is buggy around this behavior (for some reason). I guess they broke it further.

kumar303

Thanks for the quick fixes. Here is some more feedback.

kumar303 · 2018-10-18T14:27:06Z

tests/unit/amo/components/TestAddonReviewCard.js

+    expect(renderControls(root).find(FlagReviewMenu)).toHaveLength(1);
+  });
+
+  it('hides the flag button if you wrote the review', () => {


When I undo your patch, this test should fail but it does not. Try to make it fail.

Here's how I undid your patch:

diff --git a/src/amo/components/AddonReviewCard/index.js b/src/amo/components/AddonReviewCard/index.js index 13013647a..ff535434a 100644 --- a/src/amo/components/AddonReviewCard/index.js +++ b/src/amo/components/AddonReviewCard/index.js @@ -482,7 +482,7 @@ export class AddonReviewCardBase extends React.Component<InternalProps> { ) : null} {/* Do not render the "flag" button for reviews made by the current user */} - {flaggable && review && (!siteUser || siteUser.id !== review.userId) ? ( + {flaggable && review ? ( <FlagReviewMenu isDeveloperReply={this.isReply()} openerClass="AddonReviewCard-control"

Ok - here's how I think we should proceed:

The current test:

Signs the user in

Creates a review

Checks that the flag button isn't rendered

The ideal test:

Signs the user in

Creates a review

Checks that the flag button isn't rendered

Signs the user out

Checks that the flag button is rendered

Edit: This logout is just what we need!

kumar303 · 2018-10-18T14:31:07Z

tests/unit/amo/components/TestAddonReviewCard.js

@@ -576,6 +583,21 @@ describe(__filename, () => {
    expect(root.find(FlagReviewMenu)).toHaveLength(0);
  });

+  it('renders the flag button when not signed in', () => {


Actually, I just realized that you don't need this test because we already have one:

addons-frontend/tests/unit/amo/components/TestAddonReviewCard.js

Line 556 in 61424ad

it('lets you flag a review', () => {

However, I would suggest changing the spec from 'lets you flag a review' to 'shows FlagReviewMenu when signed out'

In addition to signing in the user and ensuring the "flag" button is hidden for their own replies, the test now signs the user out to ensure that all flag buttons are shown. This workflow allows unauthenticated users to see that flagging a review is possible, only after signing in.

seanprashad · 2018-10-18T17:19:18Z

Your tip made me realize that I forgot to check if the flag button would be rendered after the user logged out! I've updated this in the test and removed the old helper function I had written.

After adding back in my patch, all tests passed 🎉

kumar303

The test fails now without your patch (good!) but it needs some tweaks.

kumar303 · 2018-10-18T17:58:06Z

tests/unit/amo/components/TestAddonReviewCard.js

+
+    expect(renderControls(root).find(FlagReviewMenu)).toHaveLength(0);
+
+    store.dispatch(logOutUser());


You can delete this and the assertion below because it's a duplication of the logic covered by this test. In other words, signing out produces the exact same state as not signing in to begin with.

✅ All done

kumar303 · 2018-10-18T18:05:27Z

tests/unit/amo/components/TestAddonReviewCard.js

+  it('hides the flag button if you wrote the review', () => {
+    const originalReviewId = 123;
+    const review = signInAndDispatchSavedReview({
+      siteUserId: originalReviewId,


This variable name is confusing but I see what you are trying to do. You are letting the helper take over to set siteUserId and reviewUserId to the same value. Instead of letting the helper do this implicitly, your test should explicitly set these both to the same value so that the intention is clear.

Yeah, I didn't want to introduce another helper that wrapped the signInAndDispatchSavedReview() helper so I just used it directly. I've updated the variable naming to siteUserId so it hopefully helps us recognize that this is an authenticated user.

kumar303

This looks great. Thanks for all the quick turnarounds.

seanprashad changed the title ~~WIP - Do not render report flag for reviews by the current user~~ [WIP] Do not render report flag for reviews by the current user Oct 13, 2018

seanprashad commented Oct 13, 2018

View reviewed changes

seanprashad changed the title ~~[WIP] Do not render report flag for reviews by the current user~~ Do not render report flag for reviews by the current user Oct 13, 2018

bobsilverberg self-requested a review October 16, 2018 13:04

bobsilverberg self-assigned this Oct 16, 2018

bobsilverberg reviewed Oct 16, 2018

View reviewed changes

kumar303 suggested changes Oct 16, 2018

View reviewed changes

Sean Prashad added 3 commits October 17, 2018 13:23

Do not render report flag for reviews by the current user

45e0216

Add test for hiding flag button

839ff6d

This test is responsible for making sure that the "flag" button is hidden for replies made by the current user.

Update other tests

f0e7991

These tests are affected by the previous commit, which hides the "flag" button for reviews made by the current user.

seanprashad closed this Oct 17, 2018

seanprashad deleted the issue-6416 branch October 17, 2018 18:00

seanprashad restored the issue-6416 branch October 17, 2018 18:01

seanprashad reopened this Oct 17, 2018

Update logic for rendering the flag button

887a7d0

We should always render the "flag" button if a user is not authenticated because users should know that they have to sign in first before flagging a review.

kumar303 self-requested a review October 17, 2018 21:02

kumar303 suggested changes Oct 17, 2018

View reviewed changes

kumar303 self-requested a review October 17, 2018 21:21

Sean Prashad added 3 commits October 17, 2018 19:48

Simplify logic to show flag button

6fe536b

Revert modified unit tests

fe9900b

These were initially changed as the new implementation hid the flag button for users who weren't logged in. We can now revert these as we display the flag button for all replies that do not belong to the current user, regardless if they are signed in or not.

Add test to check if flag button is rendered for unauthenticated users

61424ad

kumar303 suggested changes Oct 18, 2018

View reviewed changes

kumar303 self-requested a review October 18, 2018 14:46

Sean Prashad added 2 commits October 18, 2018 10:48

Update spec name for clarity

26e2c05

kumar303 suggested changes Oct 18, 2018

View reviewed changes

kumar303 self-requested a review October 18, 2018 18:06

Clean up tests for clarity and remove redundant assertion

54b1274

kumar303 approved these changes Oct 18, 2018

View reviewed changes

kumar303 merged commit fe9cefc into mozilla:master Oct 18, 2018

seanprashad deleted the issue-6416 branch October 18, 2018 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not render report flag for reviews by the current user #6620

Do not render report flag for reviews by the current user #6620

seanprashad commented Oct 13, 2018 •

edited

Loading

codecov-io commented Oct 13, 2018 •

edited

Loading

seanprashad Oct 13, 2018 •

edited

Loading

kumar303 Oct 17, 2018

bobsilverberg left a comment

kumar303 commented Oct 16, 2018

kumar303 left a comment

kumar303 Oct 16, 2018

seanprashad commented Oct 17, 2018 •

edited

Loading

kumar303 left a comment

kumar303 Oct 17, 2018

kumar303 Oct 17, 2018

kumar303 Oct 17, 2018

kumar303 Oct 17, 2018

kumar303 Oct 17, 2018

seanprashad commented Oct 18, 2018 •

edited

Loading

kumar303 commented Oct 18, 2018

kumar303 left a comment

kumar303 Oct 18, 2018

seanprashad Oct 18, 2018 •

edited

Loading

kumar303 Oct 18, 2018

seanprashad commented Oct 18, 2018 •

edited

Loading

kumar303 left a comment

kumar303 Oct 18, 2018

seanprashad Oct 18, 2018

kumar303 Oct 18, 2018

seanprashad Oct 18, 2018

kumar303 left a comment

	{flaggable && review && showFlagButton ? (
	{flaggable && review && (!siteUser \|\| siteUser.id !== review.userId) ? (


		expect(renderControls(root).find(FlagReviewMenu)).toHaveLength(0);

		store.dispatch(logOutUser());

Do not render report flag for reviews by the current user #6620

Do not render report flag for reviews by the current user #6620

Conversation

seanprashad commented Oct 13, 2018 • edited Loading

codecov-io commented Oct 13, 2018 • edited Loading

Codecov Report

seanprashad Oct 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobsilverberg left a comment

Choose a reason for hiding this comment

kumar303 commented Oct 16, 2018

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanprashad commented Oct 17, 2018 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanprashad commented Oct 18, 2018 • edited Loading

kumar303 commented Oct 18, 2018

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanprashad Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanprashad commented Oct 18, 2018 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 left a comment

Choose a reason for hiding this comment

seanprashad commented Oct 13, 2018 •

edited

Loading

codecov-io commented Oct 13, 2018 •

edited

Loading

seanprashad Oct 13, 2018 •

edited

Loading

seanprashad commented Oct 17, 2018 •

edited

Loading

seanprashad commented Oct 18, 2018 •

edited

Loading

seanprashad Oct 18, 2018 •

edited

Loading

seanprashad commented Oct 18, 2018 •

edited

Loading