removed test-digest-button for Users other than Admins and Moderators #4293

uzorjchibuzor · 2018-12-13T07:45:50Z

This PR removes the Test-Digest-Button from Production, it is still present in other environments.

Kindly merge if approved.

Thanks. @publiclab/reviewers

plotsbot · 2018-12-13T07:57:37Z

	1 Message
📖	@uzorjchibuzor Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

sashadev-sky · 2018-12-13T08:03:14Z

@uzorjchibuzor Thank you! Can you post a screenshot please?

app/views/users/profile.html.erb

sashadev-sky · 2018-12-13T08:18:29Z

@uzorjchibuzor Great, thank you so much! Just need to see a screenshot

uzorjchibuzor · 2018-12-13T08:21:25Z

sashadev-sky · 2018-12-13T09:01:26Z

@uzorjchibuzor I think you may have switched your conditional around since your 1st commit: you changed it to render the button if it is in production... the issue is asking for the other way around.

Please let me know if this was intentional and if not please update!

uzorjchibuzor · 2018-12-13T09:05:31Z

@sashadev-sky, Thanks for spotting that, It wasn't intentional I am going push a new commit that corrects that.

uzorjchibuzor · 2018-12-13T09:09:03Z

sashadev-sky · 2018-12-13T09:41:02Z

@uzorjchibuzor Thank you for the edits. Your code looks good to me 👍

Sorry for confusion over the screenshot -- we always ask for screenshots of front-end updates rendered in your localhost, but I realize now that doesn't apply here because the update is for the production environment.

sashadev-sky · 2018-12-13T09:44:19Z

@jywarren @gauravano I think this PR is ready to be merged

uzorjchibuzor · 2018-12-13T09:46:25Z

Yes, I was trying to figure out a way to run passenger in production, I was having troubles until you sent the screenshot of the code section. Thank you for the review

@uzorjchibuzor Thank you for the edits. Your code looks good to me 👍 ...

@sashadev-sky

_{Sent with GitHawk}

sashadev-sky · 2018-12-13T10:02:07Z

@gauravano just saw your additional request on the original issue so I assigned you as a reviewer

…y regardless of the ENV

…r buttons

uzorjchibuzor · 2018-12-13T17:59:49Z

Screenshot for admin, moderator and user profile pages, in that order
.

uzorjchibuzor · 2018-12-13T18:03:38Z

uzorjchibuzor · 2018-12-13T18:26:55Z

@sashadev-sky Travis is failing, don’t know why. I made sure not to track any files apart from the profile.erb

_{Sent with GitHawk}

grvsachdeva · 2018-12-13T22:27:23Z

app/views/users/profile.html.erb

@@ -233,11 +233,16 @@
      </div>
    </div>
  <% end %>
+  <% if current_user.can_moderate? || current_user.role == "admin" %>


can_moderate method checks for both admin and moderator

You are directly calling current_user.mod.. and current_user.role what if there is no user currently logged in, then current_user would be containing nil. This is reason for test failure

This if condition can be removed, how about adding && current_user.can_moderate? at below if where we are checking profile_user and current_user

Thanks for the clarification. I am going make changes. I am happy I got to work on this issue

@uzorjchibuzor let me know if you need any help! I will be available over the next few hours

Also I have one more change I think you should add:

You should move the line that checks if there is a current_user above the one that checks if the current_user is a moderator or admin. Otherwise, if there is not a current_user then the 1st method will throw an error

grvsachdeva

Some changes. Thanks!

sashadev-sky

One change request from me as well! Thank you

uzorjchibuzor · 2018-12-14T16:25:31Z

uzorjchibuzor · 2018-12-14T16:47:52Z

@sashadev-sky kindly review.

_{Sent with GitHawk}

sashadev-sky · 2018-12-15T01:25:16Z

@uzorjchibuzor approved! Great job making edits. Thank you for the screenshots of the page. We are only looking for these, not screenshots of your code.

@gauravano does this cover the conditions you wanted for this button rendering? Also just a note: I was considering ways to refactor this so its less verbose, and since this pattern is all over the page I think it might be a good separate issue to open after this one is merged? Let me know what you think!

uzorjchibuzor · 2018-12-15T09:02:03Z

@uzorjchibuzor approved! Great job making edits. Thank you for the screenshots of the page. We are only looking for these, not screenshots of your code. ...

@sashadev-sky I noticed that too when I was looking at the file. I would love to be a part of that, if it was ever an issue.

_{Sent with GitHawk}

grvsachdeva · 2018-12-15T11:57:10Z

app/views/users/profile.html.erb

+    <% if @profile_user && current_user && current_user.can_moderate? && @profile_user == current_user %>
+
+      <%= form_tag "/users/test_digest_email", method: :post do %>
+        <%= submit_tag "Test Digest Email", class: "btn btn-primary" %>


Suggested change

<%= submit_tag "Test Digest Email", class: "btn btn-primary" %>

<%= submit_tag "Test Digest Email", class: "btn btn-secondary" %>

As secondary would look little lighter. What do you think?

the btn-secondary bootstrap class is grayish, but the btn-info in a bit lighter than the primary. I have gone with that.

grvsachdeva · 2018-12-15T11:59:21Z

app/views/users/profile.html.erb

@@ -233,12 +233,17 @@
      </div>
    </div>
  <% end %>
+


Please add a <br /> here to add some space between buttons. And, please remove the extra spaces like line #239, 243, 245, 256. Thanks!

@uzorjchibuzor the functionality is working great, please do the above changes to optimize design a little. Also, if you have idea for more clean design than you can try.

Thanks!

I will implement the changes suggested and also thoroughly look at the code to what can be refactored

grvsachdeva · 2018-12-15T12:01:57Z

Hi @sashadev-sky I would love to hear ideas about shrinking the repeated code to a function or so, feel free to open a new issue. And, @uzorjchibuzor you are welcome to help on this issue. What do you think @sashadev-sky?

Thanks!

sashadev-sky · 2018-12-15T20:10:32Z

@gauravano I am not at my computer right now but maybe we can add a new method to application controller or at the top of the erb file? I will collaborate with @uzorjchibuzor and we will decide and open issue!

@uzorjcibuzor is Sunday ok?

uzorjchibuzor · 2018-12-15T20:32:03Z

Sunday is good

grvsachdeva

Nice work @uzorjchibuzor !

grvsachdeva · 2018-12-16T17:09:17Z

Merged 🎉 💯 🎈

sashadev-sky · 2018-12-17T03:22:52Z

@uzorjchibuzor would you be able to get on Gitter?

uzorjchibuzor · 2018-12-17T05:16:34Z

I have a gitter account, what is the name of the community there?

sashadev-sky · 2018-12-17T05:29:31Z

@uzorjchibuzor if you click 'Gitter' in my above comment it should take you directly to the public lab page

uzorjchibuzor · 2018-12-17T05:56:44Z

I have joined the room, installed the app too as I am not going to be on the computer for a long time today. @sashadev-sky

sashadev-sky · 2018-12-17T06:09:30Z

@uzorjchibuzor ok great, I sent you a message so that we can coordinate 👍

…publiclab#4293) * removed test-digest-button from production environment * added space between conditionals * improved code readability * indented tags * corrected error in conditional statement * restricted the Test-Digest-Button to be viewed by Admins and Mods only regardless of the ENV * changed the style of the test button to btn-primary like all the other buttons * added a space between conditionals and tags * removed redundant if statement * adding && current_user.can_moderate? * removed extra spaces to make code cleaner * Button size changed

removed test-digest-button from production environment

7f5481b

sashadev-sky reviewed Dec 13, 2018

View reviewed changes

app/views/users/profile.html.erb Outdated Show resolved Hide resolved

app/views/users/profile.html.erb Outdated Show resolved Hide resolved

added space between conditionals

0e33a8a

uzorjchibuzor added 2 commits December 13, 2018 09:18

improved code readability

08e5c6d

indented tags

4d23f98

corrected error in conditional statement

2f91a6f

sashadev-sky approved these changes Dec 13, 2018

View reviewed changes

sashadev-sky requested a review from grvsachdeva December 13, 2018 10:00

uzorjchibuzor added 2 commits December 13, 2018 18:47

restricted the Test-Digest-Button to be viewed by Admins and Mods onl…

b7101ba

…y regardless of the ENV

changed the style of the test button to btn-primary like all the othe…

cb8081a

…r buttons

uzorjchibuzor changed the title ~~removed test-digest-button from production environment~~ removed test-digest-button for Users other than Admins and Moderators Dec 13, 2018

added a space between conditionals and tags

b7d3c61

grvsachdeva reviewed Dec 13, 2018

View reviewed changes

grvsachdeva requested changes Dec 13, 2018

View reviewed changes

sashadev-sky requested changes Dec 14, 2018

View reviewed changes

removed redundant if statement

27cc723

sashadev-sky approved these changes Dec 15, 2018

View reviewed changes

grvsachdeva reviewed Dec 15, 2018

View reviewed changes

grvsachdeva added the in-progress label Dec 15, 2018

uzorjchibuzor and others added 2 commits December 16, 2018 08:14

removed extra spaces to make code cleaner

25f53e7

Button size changed

3ad01a5

ghost assigned grvsachdeva Dec 16, 2018

ghost added the in progress label Dec 16, 2018

grvsachdeva assigned uzorjchibuzor and unassigned grvsachdeva Dec 16, 2018

grvsachdeva removed in progress in-progress labels Dec 16, 2018

grvsachdeva approved these changes Dec 16, 2018

View reviewed changes

grvsachdeva merged commit 8843717 into publiclab:master Dec 16, 2018

uzorjchibuzor deleted the hide-test-button branch December 17, 2018 05:55

uzorjchibuzor restored the hide-test-button branch December 17, 2018 05:55

	<%= submit_tag "Test Digest Email", class: "btn btn-primary" %>
	<%= submit_tag "Test Digest Email", class: "btn btn-secondary" %>

removed test-digest-button for Users other than Admins and Moderators #4293

removed test-digest-button for Users other than Admins and Moderators #4293

Conversation

uzorjchibuzor commented Dec 13, 2018

plotsbot commented Dec 13, 2018 • edited

sashadev-sky commented Dec 13, 2018

sashadev-sky commented Dec 13, 2018

uzorjchibuzor commented Dec 13, 2018

sashadev-sky commented Dec 13, 2018

uzorjchibuzor commented Dec 13, 2018

uzorjchibuzor commented Dec 13, 2018

sashadev-sky commented Dec 13, 2018

sashadev-sky commented Dec 13, 2018

uzorjchibuzor commented Dec 13, 2018

sashadev-sky commented Dec 13, 2018

uzorjchibuzor commented Dec 13, 2018

uzorjchibuzor commented Dec 13, 2018

uzorjchibuzor commented Dec 13, 2018

grvsachdeva Dec 13, 2018

Choose a reason for hiding this comment

uzorjchibuzor Dec 13, 2018

Choose a reason for hiding this comment

sashadev-sky Dec 14, 2018 • edited

Choose a reason for hiding this comment

grvsachdeva left a comment

Choose a reason for hiding this comment

sashadev-sky left a comment

Choose a reason for hiding this comment

uzorjchibuzor commented Dec 14, 2018

uzorjchibuzor commented Dec 14, 2018

sashadev-sky commented Dec 15, 2018

uzorjchibuzor commented Dec 15, 2018

grvsachdeva Dec 15, 2018

Choose a reason for hiding this comment

uzorjchibuzor Dec 16, 2018

Choose a reason for hiding this comment

grvsachdeva Dec 15, 2018

Choose a reason for hiding this comment

grvsachdeva Dec 15, 2018

Choose a reason for hiding this comment

uzorjchibuzor Dec 15, 2018

Choose a reason for hiding this comment

grvsachdeva commented Dec 15, 2018

sashadev-sky commented Dec 15, 2018

uzorjchibuzor commented Dec 15, 2018 via email • edited

grvsachdeva left a comment

Choose a reason for hiding this comment

grvsachdeva commented Dec 16, 2018

sashadev-sky commented Dec 17, 2018

uzorjchibuzor commented Dec 17, 2018

sashadev-sky commented Dec 17, 2018

uzorjchibuzor commented Dec 17, 2018

sashadev-sky commented Dec 17, 2018

plotsbot commented Dec 13, 2018 •

edited

sashadev-sky Dec 14, 2018 •

edited

uzorjchibuzor commented Dec 15, 2018 via email •

edited