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.

Sign up for GitHub

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

Jump to bottom

Include Emails of Banned Users in Moderator Email Search #6962

Closed
5 tasks
skilfullycurled opened this issue Dec 13, 2019 · 50 comments · Fixed by #7137
Closed
5 tasks

Include Emails of Banned Users in Moderator Email Search #6962

skilfullycurled opened this issue Dec 13, 2019 · 50 comments · Fixed by #7137
Labels
feature explains that the issue is to add a new feature first-timers-only They need to be well-formatted using the First-timers_Issue_Template. help wanted requires help by anyone willing to contribute

Comments

@skilfullycurled
Copy link
Contributor

skilfullycurled commented Dec 13, 2019 •
edited
Loading

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

We know that the process of creating a pull request is the biggest barrier for new contributors. This issue is for you 💝

If you have contributed before, consider leaving this one for someone new, and looking through our general help wanted issues. Thanks!

🤔 What you will need to know.

Moderators have access to a feature that allows them to lookup profiles by email. During a recent attempt to unban a mistakenly banned user (#6883) it was discovered that this feature does not include the emails of banned users.

In discussion with the moderation team, there are some good use cases for including these emails:

  1. if a user created their account a long time ago, they may not remember there username (or remember it correctly). In those instances, finding a profile by email would be the only recourse.

  2. Including banned users may eliminate a round of back and forth if the person emailing the moderation team is using the email address they signed up with.

The current solution (related #7109)is to create a check box that when toggled will add banned from 2015 onward to the search. We also need to add some text explaining what the check box does and the impact that using it may have on the site.

NOTE: See the update section below for a link to the post in this conversation where the steps are located to complete the FTO.

📋 Step by Step

  • 🙋 Claim this issue: Comment below. If someone else has claimed it, ask if they've opened a pull request already and if they're stuck -- maybe you can help them solve a problem or move it along!

  • 📝 Update
    The steps needed to complete the FTO can be found in the conversation below at this post.

See this page for some help in taking your first steps!

  • 💾 Commit your changes

  • 🔀 Start a Pull Request. There are two ways how you can start a pull request:

  1. If you are familiar with the terminal or would like to learn it, here is a great tutorial on how to send a pull request using the terminal.

  2. You can also edit files directly in your browser and open a pull request from there.

  • 🏁 Done Ask in comments for a review :)

Please keep us updated

💬⏰ - We encourage contributors to be respectful to the community and provide an update within a week of claiming a first-timers-only issue. We're happy to keep it assigned to you as long as you need if you update us with a request for more time or help, but if we don't see any activity a week after you claim it we may reassign it to give someone else a chance. Thank you in advance!

If this happens to you, don't sweat it! Grab another open issue.

Is someone else already working on this?

🔗- We encourage contributors to link to the original issue in their pull request so all users can easily see if someone's already started on it.

👥- If someone seems stuck, offer them some help! Otherwise, take a look at some other issues you can help with. Thanks!

🤔❓ Questions?

Leave a comment below!
@skilfullycurled skilfullycurled added the feature explains that the issue is to add a new feature label Dec 13, 2019
@skilfullycurled
Copy link
Contributor Author

Regarding the number of spam users slowing things down:

We have at least over 350,000 actual users (see here) but that's from 2013 and things seem pretty good. Did we not require email back then?

I trust you could do an actual query but as a rough estimate: I've been collecting moderation emails since 5/1/19 and we've only marked ~1800 users as spam (and banned them as a consequence). So, extrapolating from that (admittedly a flawed idea) let's say conservatively, we have ~4000 spam users a year.

Perhaps we could limit the inclusion dates to some number of years that would keep everything speedy. Or is there a slow down due to the fact that now you have to add in another query parameter?

I don't think the feature needs to capture all edge cases, just enough that it's worthwhile to have it. I haven't been on the moderation list for long enough to know how long that should be. Do other moderators have any thoughts on that?

@jywarren
Copy link
Member

jywarren commented Jan 2, 2020

Just thinking about how to warn people about this, the checkbox could have a helpful warning like [ ] include banned accounts (careful: the huge # of accounts can make this slow the whole site down)

We could have the checkbox add a POST parameter like banned=true and then on line 59 of this code segment, switch off that conditional accordingly:

plots2/app/controllers/admin_controller.rb

Lines 63 to 75 in 0f94ede

def useremail
if logged_in_as(['admin', 'moderator'])
if params[:address]
# address was submitted. find the username(s) and return.
@address = params[:address]
@users = User.where(email: params[:address])
.where(status: [1, 4])
end
else
# unauthorized. instead of return ugly 403, just send somewhere else
redirect_to '/dashboard'
end
end

@jywarren
Copy link
Member

jywarren commented Jan 2, 2020

If this sounds reasonable, @skilfullycurled, we can mark it as an FTO candidate and hand it off to folks for implementation? ❤️ Happy new year!

@skilfullycurled
Copy link
Contributor Author

Happy New Year!

For sure, did you need me to change anything on this issue or do you have the power to edit it?

A few suggestions:

  1. Would it help to put a time constraint on the search or would that slow the process down even more? There was a period of time when we got 350,000+ signups in 2013 and the probability that the needle is in that haystack is very close to zero.

  2. I'd give the warning a use case or condition to satisfy before doing it, and a reminder to be patient. This is an edge case since current moderators already know the use case, but it's a small addition that will have two positive effects: 1) it shouldn't be their first option but 2) it's an acceptable option and they shouldn't shy away from using it in particular instances. When using a "search all" function, I think it's common for people to try it and then realize that the time it's taking isn't worth it so they cancel the request.

As a verbose example:

[ ] include banned accounts (careful: Please use only in cases where the user is already banned or you are unable to find the user through the normal search first. The huge # of accounts can make this slow the whole site down. Please be patient as it loads.)

  1. I had another idea since I last filed this which would also make a good FTO. We could also add something using the body parameter in the mailto such as:

"Please make sure to include your username and the email address you used to sign up for the site."

<a href="mailto:moderators@publiclab.com?body=Please make sure to include your username and the email address you used to sign up for the site">moderators@publiclab.com</a>

The overall goal as I see it is to reduce the back and forth between the moderator and the user while also making the need to search all of the users a very rare instance. So I think anything we can that's simple and prevents needing to use the search feature in the first place is a worthwhile addition.

@jywarren
Copy link
Member

jywarren commented Jan 3, 2020 via email

@skilfullycurled
Copy link
Contributor Author

If you think it will make a difference, we can shave off (possibly as much as) another ~36,000, then you can start at 2015. That's not from actual data, I'm extrapolating but I think it's a reasonable one. I mean hey, you're a decade old now (and it's a new decade) so you're allowed to do things by 5's now! I leave that to you, I have no opinion, just offering some info albeit not based entirely on fact. So I guess that makes it an opinion. Anyway, you get it.

Will do on the FTO.

@skilfullycurled
Copy link
Contributor Author

Wait! I didn't mean to close it...clicked on the wrong button. Unless that's okay that I closed it. Seems like we've come to an end for the time being

@jywarren jywarren reopened this Jan 3, 2020
@jywarren
Copy link
Member

jywarren commented Jan 3, 2020

No, let's go ahead on this - I think then the changes would be to change lines 68-9 of this:

plots2/app/controllers/admin_controller.rb

Lines 63 to 75 in 0f94ede

def useremail
if logged_in_as(['admin', 'moderator'])
if params[:address]
# address was submitted. find the username(s) and return.
@address = params[:address]
@users = User.where(email: params[:address])
.where(status: [1, 4])
end
else
# unauthorized. instead of return ugly 403, just send somewhere else
redirect_to '/dashboard'
end
end

to something like:

       if params[:include_banned]
         @users = User.where(email: params[:address]) 
           .where('created_at > (?)', DateTime.new(2015)) # since 2015, whether banned or not
       else
         @users = User.where(email: params[:address]) 
           .where(status: [1, 4]) 
       end

And a template change to add a checkbox to this:

plots2/app/views/admin/useremail.html.erb

Lines 7 to 13 in d251b5f

<%= form_tag do %>
<%= text_field_tag(:address, value = @address) %>
<%= button_tag(:search, type: 'submit', class: "btn btn-primary") do %>
<i class="fa fa-search fa fa-white"></i>
<% end %>
</button>
<% end %>

like:

<label>
  <input type="checkbox" id="myCheck" name="include_banned" />
  Include banned accounts after 2015 (careful: can result in server slowness for all users)
</label>

I think this is ready to be made into an FTO!

This has been marked as a good candidate for becoming a first-timers-only issue like these, meaning that it's simple, self-contained, and with some extra formatting, could be a great entry point for a new contributor. If you're familiar enough with this code, please consider reformatting or reposting it as a first-timers-only issue, and then ping @publiclab/reviewers to get it labelled. Or, if this is not your first time, try to solve it yourself!

@jywarren jywarren added fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet help wanted requires help by anyone willing to contribute labels Jan 3, 2020
@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 4, 2020 •
edited
Loading

Ah, you know, I remember now that there's a template for FTO's. Do we typically convert the original issue (meaning this one) by changing the original text and filling it in with the new information we have from the discussion or do we need a new one?

@skilfullycurled skilfullycurled mentioned this issue Jan 4, 2020
Closed
5 tasks
@jywarren
Copy link
Member

jywarren commented Jan 5, 2020 via email

@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 5, 2020 •
edited
Loading

Edit: Nevermind, I see now that when you mark it as an FTO candidate, it says that you reformat or re-post. Reformatting now.

I'm still not clear: I meant in regards to this one, when you said it's ready to be an FTO, should I make a new one or just fix the formatting on this issue so it matches the FTO template. The other one that I wrote was a different FTO for the mailto text.

@skilfullycurled
Copy link
Contributor Author

I'm just going to go ahead and do it and we can "fix it in post" as they say.

@jywarren
Copy link
Member

jywarren commented Jan 6, 2020

Sorry - yeah, i don't have a strong opinion on reformatting vs making a new one. If you want to keep people in the notifications thread you can use the same issue. But not a big deal either way! Thank you!!

@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 6, 2020 •
edited
Loading

@jywarren, I reformatted the original post. Since you already had the steps in the conversation, I simply linked to that post in the "update" section on the checklist. I think it's officially ready for a label upgrade!

@shipcy, if you're interested in taking it on, go head and claim it!

Thanks everyone!

Edit: Actually, let's just reserve it for @shipcy and wait for them to respond. They had expressed interest in the related FTO which was already taken.

@jywarren jywarren added first-timers-only They need to be well-formatted using the First-timers_Issue_Template. and removed fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet labels Jan 6, 2020
@skilfullycurled
Copy link
Contributor Author

@shipcy, can you say a bit more about your experience so I know where to begin to help you?

What have been your experiences with Github? Have you worked with Github with your own work? Have you contributed to another repository before?

@shipcy
Copy link
Contributor

shipcy commented Jan 6, 2020

@skilfullycurled I'm totally new to open source. I've not that much idea about the contributions. But I'll try my best to apply my skills here and will try to solve issues. And I haven't contributed to any repository yet. Please guide me.

@skilfullycurled
Copy link
Contributor Author

That's actually perfect @shipcy because I've been wanting to update the and see if people can follow it anyway.

BTW: Also, introduce yourself here. This is our weekly community check-in. Each week there is a new question that is asked. Often it is centered around what people are working on, but sometimes I just use it to say "hello". Let everyone know you're working on your first contribution. You'll get an enthusiastic greeting, and people will generally let you know if they're available for any questions!

Okay, let me take some screenshots, and update the tutorial. Be right back!

@skilfullycurled
Copy link
Contributor Author

@shipcy, I was looking at your profile, and it seems like you do know how to fork the repository. If so, then you can go ahead and fork plots2. If not, then just wait and I'll have an image up in a few minutes.

@shipcy
Copy link
Contributor

shipcy commented Jan 6, 2020

Hey, everyone! My name is Shipra Choudhary. I'm currently a junior pursuing Computer Science and Engineering from India. My skill set includes HTML, CSS, Bootstrap, Javascript, JQuery, C++, MySQL, etc. And I'm new to open source and I'm working on my first contribution.

@skilfullycurled
Copy link
Contributor Author

@shipcy, I'm so sorry. I gave you the wrong link.

(sigh)

Follow this link and introduce yourself here.

@shipcy
Copy link
Contributor

shipcy commented Jan 6, 2020

@skilfullycurled I have given my introduction.

@skilfullycurled
Copy link
Contributor Author

Great! Okay, give me a bit, I need to check on some things with folks. In the meantime, you can fork the repository.

@shipcy
Copy link
Contributor

shipcy commented Jan 6, 2020

@skilfullycurled I have done it.

@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 6, 2020 •
edited
Loading

You did great! You went really far without very good directions or instructions. Well done.

A few things:

  1. Don't forget the second part which is changing the template to add the check box (see above). However, we should get more specific on where that code goes first. @jywarren, when you have a moment, can you let us know what line the html goes in the template in your comment above?
  1. I didn't realize that we hadn't given you any lines for where the new code goes. The end result should look like the following below, so you can now replace the entire def useremail function instead of figuring out where it gets inserted. It should still be around line 63.
  def useremail
    if logged_in_as(['admin', 'moderator'])
      if params[:address]
        # address was submitted. find the username(s) and return.
        @address = params[:address]
        if params[:include_banned]
          @users = User.where(email: params[:address]) 
          .where('created_at > (?)', DateTime.new(2015)) # since 2015, whether banned or not
        else
          @users = User.where(email: params[:address]) 
          .where(status: [1, 4]) 
        end
      else
        # unauthorized. instead of return ugly 403, just send somewhere else
        redirect_to '/dashboard'
      end
    end
  end

You can make the changes to your current branch and I believe the pull request you created should update automatically with the new commits. However, I'd cancel the pull request and start over because I have two other suggestions. It's your choice, truly.

  1. You've named your branch shipcy-patch-1. I believe "patches" are for fixes, not new features, so give your branch a name that describes the feature such as scipcy-banned-users.
  1. Make sure to name your commits with more description as well. Instead of "updated admin_controller.rb", "modified useremail function to include banned users". Then, when you modify the useremail.html.erb your commit could be "added checkbox to include banned users". You want to get specific enough that someone else could easily find your commit from the giant list of commits if they had to.

@skilfullycurled
Copy link
Contributor Author

PS: This is really helpful by the way. These errors are really on us and our directions so it's great to be able to see where our directions are failing!

@jywarren
Copy link
Member

jywarren commented Jan 6, 2020

Hi! just noting if it'd be helpful just be sure to ping me @jywarren with any questions. You're doing great!

@skilfullycurled
Copy link
Contributor Author

@jywarren, you might have missed my question in the giant text, but when you have a moment, can you let us know what line the html goes in the template in your comment above?

@jywarren
Copy link
Member

jywarren commented Jan 6, 2020

Oh sorry! Yes, i think it'd go just before line 13, but you may want to wrap it in <p> </p> tags depending on how it ends up looking!

@shipcy
Copy link
Contributor

shipcy commented Jan 7, 2020

@skilfullycurled I've done the def useremail function changes and made another pull request.

@shipcy
Copy link
Contributor

shipcy commented Jan 7, 2020

@skilfullycurled I have also completed the second part which is changing the template to add the checkbox and made a pull request.

@shipcy
Copy link
Contributor

shipcy commented Jan 7, 2020

@skilfullycurled and @jywarren I wanna know the one thing that why are we using % inside HTML tags like <% if @address %> ?

@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 7, 2020 via email

@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 7, 2020 via email

@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 7, 2020 •
edited
Loading

@shipcy, I've got to bring someone else into the conversation because for some reason your pull requests aren't showing up in the plots2 pull requests. Hold tight!

Also, I could have sworn you wrote a comment asking me what I meant by "question at the top". I'm not sure which one your referring to. Let me know!

@jywarren, when you have a chance, could you take a look at what is causing the pull requests to not appear in plots2?

@shipcy
Copy link
Contributor

shipcy commented Jan 7, 2020

@skilfullycurled why my pull requests aren't showing? Maybe any fault from my side

@skilfullycurled
Copy link
Contributor Author

@shipcy, yes, I noticed. In my comment above yours I asked for people to help out. Are you receiving notifications via email when the thread is updated with new comments? Because we're all leaving comments at different times (sometimes at the same time!), it's easy for one to get out of sync with the conversation. If you haven't already, be sure to take a look at what's come in since you last posted.

@skilfullycurled
Copy link
Contributor Author

skilfullycurled commented Jan 7, 2020 •
edited
Loading

Pinging @publiclab/connectors. If anyone has some time to dedicate to helping today, I'd appreciate it, I'll out more than usual. @shipcy is doing quite a good job and I want to make sure they still has the support they need (!) and don't have to wait too long for help.

@nstjean
Copy link
Contributor

nstjean commented Jan 7, 2020

Hi @shipcy ! I took a look at your pull requests and it looks like they were created on your fork instead of the master branch. So all you need to do is go to https://github.com/publiclab/plots2/pulls and create the pull request there. Then it will show up as not just something in your personal fork, but for the public lab repository master branch!

After you click on the New Pull Request button you're going to click on the link compare between forks. make sure the left side has the publiclab/plots2/master and on the right side you will select your fork shipcy/plots2 and then select the branch your changes are in.

FireShot Capture 181 - Comparing publiclab_master nstjean_master · publiclab_plots2 - github com

@shipcy
Copy link
Contributor

shipcy commented Jan 7, 2020

@nstjean I can't see the base repository and head repository.
Screenshot 2020-01-07 at 10 48 32 PM

@nstjean
Copy link
Contributor

nstjean commented Jan 7, 2020

Click on the blue link "compare across forks" and it should show up!

@shipcy
Copy link
Contributor

shipcy commented Jan 7, 2020

@nstjean thank you so much. I've done this.

@nstjean
Copy link
Contributor

nstjean commented Jan 7, 2020

Excellent! I see them now!

@shipcy
Copy link
Contributor

shipcy commented Jan 7, 2020

@skilfullycurled and @nstjean what to do next? Please guide me.

@nstjean
Copy link
Contributor

nstjean commented Jan 7, 2020

You now just have to wait for your pull request to be reviewed. :) I will check to make sure the page looks ok and either post suggestions for changes or approval!

This was referenced Jan 7, 2020
Closed
Merged
@shipcy
Copy link
Contributor

shipcy commented Jan 9, 2020

@skilfullycurled my pull request is having issues. And I don't know what to do next. Please help me out.

@skilfullycurled
Copy link
Contributor Author

Hey @scipcy! Sorry to have been gone yesterday, I had a project I needed to give some love to.

Okay!

First things first.

Once you have a pull request submitted, move the conversation over there, that way we can keep all of the issues that are impeding the request on that that page.

@nstjean is taking a look right now, and I'll join in, too!

@shipcy
Copy link
Contributor

shipcy commented Jan 10, 2020

Thanks @skilfullycurled

@nstjean nstjean mentioned this issue Jan 10, 2020
Merged
@skilfullycurled
Copy link
Contributor Author

Hi, just wanted to notify everyone that I tried out the interface and it was actually very speedy, in the order of seconds. The user I was looking up only signed on a few weeks ago, and it's almost midnight EST, but still, it was nice to know the site won't implode for the most frequent use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature explains that the issue is to add a new feature first-timers-only They need to be well-formatted using the First-timers_Issue_Template. help wanted requires help by anyone willing to contribute
Projects
None yet
Milestone
No milestone
4 participants
@jywarren @skilfullycurled @nstjean @shipcy