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

Added email verification (fixes #1996) #2164

Closed
wants to merge 0 commits into from

Conversation

Ankit-Singla
Copy link

@Ankit-Singla Ankit-Singla commented Jan 30, 2018

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

  • all tests pass -- rake test:all
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request is descriptively named with #number reference back to original issue

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

@PublicLabBot
Copy link

PublicLabBot commented Jan 30, 2018

12 Errors
🚫 There was a test failure at: Failure: test_about_ability_of_power_tags_to_exclude_tags_like_activities:foo!foo1 [/app/test/unit/node_shared_test.rb:157]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_about_ability_of_power_tags_to_exclude_tags_like_notes:test!awesome [/app/test/unit/node_shared_test.rb:125]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_about_ability_of_power_tags_to_exclude_tags_like_questions:foo!answered [/app/test/unit/node_shared_test.rb:145]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_about_ability_of_power_tags_to_exclude_tags_like_questions:foo!foo1 [/app/test/unit/node_shared_test.rb:135]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_about_ability_of_power_tags_to_exclude_tags_like_upgrades:foo!foo1 [/app/test/unit/node_shared_test.rb:166]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_that_NodeShared_can_be_used_to_convert_doubled_short_codes_like_[notes:activity:spectrometer]into_tables_which_list_notes_with_the_tagactivity:spectrometer(Minitest::Result) [/app/test/unit/node_shared_test.rb:25]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_that_NodeShared_can_be_used_to_convert_short_codes_like_[activities:foo]_into_tables_which_list_activity_notes(Minitest::Result) [/app/test/unit/node_shared_test.rb:47]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_that_NodeShared_can_be_used_to_convert_short_codes_like_[notes:foo]_into_tables_which_list_notes(Minitest::Result) [/app/test/unit/node_shared_test.rb:10]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_that_NodeShared_can_be_used_to_convert_short_codes_like_[questions:foo]_into_tables_which_list_questions(Minitest::Result) [/app/test/unit/node_shared_test.rb:35]: Expected: 5 Actual: 4
🚫 There was a test failure at: Failure: test_that_NodeShared_can_be_used_to_convert_short_codes_like_[upgrades:latest]_into_tables_which_list_upgrade_notes(Minitest::Result) [/app/test/unit/node_shared_test.rb:57]: Expected: 5 Actual: 4
🚫 There was a test error at: Failure: test_about_ability_of_power_tags_to_exclude_tags_like_people:organizer!foo1 [/usr/local/bundle/gems/actionview-4.1.16/lib/action_view/template.rb:297]: SyntaxError: /app/app/views/grids/_people.html.erb:23: syntax error, unexpected keyword_do, expecting keyword_then or ‘;’ or ‘\n’ …r.append=( if(users.length>0)do users.length-10 + “more” els… … ^ /app/app/views/grids/_people.html.erb:23: syntax error, unexpected keyword_else, expecting ‘)’ …o users.length-10 + “more” else “less” end );@output_buffer…. … ^ /app/app/views/grids/_people.html.erb:23: syntax error, unexpected keyword_end, expecting ‘)’ …gth-10 + “more” else “less” end );@output_buffer.safe_append… … ^ /app/app/views/grids/_people.html.erb:28: syntax error, unexpected keyword_end, expecting ‘)’ ‘.freeze; end ^ /app/app/views/grids/_people.html.erb:55: syntax error, unexpected keyword_ensure, expecting ‘)’ /app/app/views/grids/_people.html.erb:57: syntax error, unexpected keyword_end, expecting ‘)’ app/models/concerns/node_shared.rb:277:in block in people_grid' app/models/concerns/node_shared.rb:253:in gsub’ app/models/concerns/node_shared.rb:253:in people_grid' <a href='https://github.com/Ankit-Singla/plots2/tree/master/test/unit/node_shared_test.rb#L180'>test/unit/node_shared_test.rb:180</a>:in block in '
🚫 There was a test error at: Failure: test_that_NodeShared_can_be_used_to_convert_short_codes_like_[people:organizer]into_maps_which_display_notes,_but_only_those_tagged_with“organizer”(Minitest::Result) [/usr/local/bundle/gems/actionview-4.1.16/lib/action_view/template.rb:297]: SyntaxError: /app/app/views/grids/_people.html.erb:23: syntax error, unexpected keyword_do, expecting keyword_then or ‘;’ or ‘\n’ …r.append=( if(users.length>0)do users.length-10 + “more” els… … ^ /app/app/views/grids/_people.html.erb:23: syntax error, unexpected keyword_else, expecting ‘)’ …o users.length-10 + “more” else “less” end );@output_buffer…. … ^ /app/app/views/grids/_people.html.erb:23: syntax error, unexpected keyword_end, expecting ‘)’ …gth-10 + “more” else “less” end );@output_buffer.safe_append… … ^ /app/app/views/grids/_people.html.erb:28: syntax error, unexpected keyword_end, expecting ‘)’ ‘.freeze; end ^ /app/app/views/grids/_people.html.erb:55: syntax error, unexpected keyword_ensure, expecting ‘)’ /app/app/views/grids/_people.html.erb:57: syntax error, unexpected keyword_end, expecting ‘)’ app/models/concerns/node_shared.rb:277:in block in people_grid' app/models/concerns/node_shared.rb:253:in gsub’ app/models/concerns/node_shared.rb:253:in people_grid' <a href='https://github.com/Ankit-Singla/plots2/tree/master/test/unit/node_shared_test.rb#L99'>test/unit/node_shared_test.rb:99</a>:in block in '
1 Warning
⚠️ This pull request cannot be merged yet due to merge conflicts. Please resolve them – probably by rebasing – and ask for help (in the comments, or in the chatroom if you get into trouble!.
3 Messages
📖 @Ankit-Singla 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.
📖 Your pull request is on the master branch. Please make a separate feature branch) with a descriptive name like new-blog-design while making PRs in the future.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

@namangupta01
Copy link
Member

@Ankit-Singla Please also mention the issue number in the pr comment like fixes #1996

@namangupta01
Copy link
Member

Otherwise it seems good! Nice @Ankit-Singla 👍

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 30, 2018

@Ankit-Singla Thanks for the pr. Can you please add a test? So that we can verify that if a user is trying to make an account through an email like anskit@gmail then he will not be able to.
Everything else looks good

@Ankit-Singla Ankit-Singla changed the title Added email verification Added email verification (fixes #1996) Jan 30, 2018
@seafr
Copy link
Member

seafr commented Jan 30, 2018

The period after the + sign will match any character and that's not what we want. I think a backslash is needed here. You can also append an 'i' to make it case insensitive.

@seafr
Copy link
Member

seafr commented Jan 30, 2018

Regarding testing, please see the last test in test/unit/user_test.rb.

@Ankit-Singla
Copy link
Author

I have added the corresponding unit test as well and it runs fine. All three tasks have been completed. I have also merged the three commits into a single one.

@SidharthBansal
Copy link
Member

Please remove the changes which you have pushed just now. That needs to be tackled in other pr

Copy link
Member

@SidharthBansal SidharthBansal left a comment

Choose a reason for hiding this comment

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

Please remove the commits which you pushed just now for the issue #2150. Make a new pr for that issue

@Ankit-Singla
Copy link
Author

@SidharthBansal how can I do that? where do I need to go

@@ -20,7 +20,7 @@
</tr>
<% if index == 9 %>
<tr class="show-all">
<td><a>Show <%= users.length - 10 %> more <b class="caret"></b></a></td>
<td><a>Show <%= if(users.length>0)do users.length-10 + "more" else "less" end %> <b class="caret"></b></a></td>
Copy link
Member

Choose a reason for hiding this comment

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

if is followed by do here. I think you want to use then instead of do. Another option is to use the conditional operator.

@ViditChitkara
Copy link
Member

Hi Ankit, you can use git reset hard to reset the HEAD to desired commit!!
https://stackoverflow.com/questions/448919/how-can-i-remove-a-commit-on-github

@seafr
Copy link
Member

seafr commented Jan 31, 2018

@ViditChitkara correct me if I'm wrong, but he will then lose all changes that come after the new HEAD.

@SidharthBansal
Copy link
Member

@Ankit-Singla I guess you have #2164 and #2169 for the same issue #1996 So this one is duplicate of #2169.
Isn't it? Can you please close this one as we have the other one with the same work.
Thanks

@SidharthBansal
Copy link
Member

Also you have the work for #2150 in the #2184. So, it is better to close this one. That code is also duplicate here

@Ankit-Singla
Copy link
Author

I have closed this pull request as I made two PRs for the same issue, by mistake.
I am new to using github, and I was not getting the hang of it initially. Sorry for the inconvenience. Going ahead, I'll be extra careful.

@jywarren
Copy link
Member

jywarren commented Feb 2, 2018

Hi, thanks everyone for helping to untangle this, and @Ankit-Singla please don't worry, this can happen and we're here to help you figure it out!

Part of this is because you're developing on your master branch. Using feature branches can help keep your submissions separate and is cleaner. See this for some guidance on this workflow, under "Github Flow":

https://publiclab.org/wiki/developers#Resources

For now, if you're sure you have these last few commits represented in other PRs, you should be good to do as @ViditChitkara points out to remove them. However, you may want to do git checkout -b new-branch-name with some unique name instead of unique-branch-name, and submit a new PR that's not based on your master branch.

Does that make sense? Once we get this sorted out, we can tackle some of the changes @seafr has requested, which I think are good.

Thanks, and no worries! We'll figure it out! Thanks for your contribution!

@Ankit-Singla
Copy link
Author

@jywarren Thanks a lot for your support. I have corrected what I did wrong, and have opened another pull request #2169 from a uniquely named feature branch. All checks have passed, so could you please review the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants