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

User should not be able to create Oauth based user_tags #2737

Merged
merged 5 commits into from
Jun 3, 2018

Conversation

SidharthBansal
Copy link
Member

@SidharthBansal SidharthBansal commented May 20, 2018

Part of #2388

@PublicLabBot
Copy link

PublicLabBot commented May 20, 2018

1 Message
📖 @SidharthBansal 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

@SidharthBansal
Copy link
Member Author

image
It is working properly.
@jywarren where do I need to write documentation for Oauth, Omniauth gems and figaro gem. That I forget to write in previous commits.I could not find the exact place where the information will suit best.
Thanks

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Great !

@jywarren
Copy link
Member

This looks super! I do wonder if we should leave these on until we enable the OAuth system -- we could comment out the line where it checks and just put true for now -- because people are currently still adding their accounts to their profiles, without Oauth linking. OR we could have a more specialized tag like oa-github:_____ so that we don't collide with this use case. Maybe the second solution makes more sense and we can merge this feature without commenting out?

Great work!!!

@jywarren
Copy link
Member

Also perhaps we can have the message say This tag is used for associating a Twitter account. Click here to read more and/or link to where you can add your twitter account? That way this moment is actually also an invitation to begin doing this. But if that part is not implemented yet, we can direct to a placeholder page like publiclab.org/oauth where we can make a wiki page with some explanation. What do you think?

@SidharthBansal
Copy link
Member Author

SidharthBansal commented May 24, 2018

I am in favour of second option 'oa-github:____'. I have made it oauth-github:___ so that it will be more clear.

@SidharthBansal
Copy link
Member Author

SidharthBansal commented May 24, 2018

@jywarren The user_tag needs to be changed before I implement the OAuth. The main model which I will be using for the OAuth is user_tag. So, we can't let any changes remaining in user_tag. We can't comment them for now.

@SidharthBansal
Copy link
Member Author

SidharthBansal commented May 27, 2018

image
I have made the required changes as suggested by you in the pr. Please tell me other patches.
Thanks

@jywarren
Copy link
Member

This looks great. Seems like we just need to resolve the octokit bug now, in #2738 (comment) -- see if you can coordinate with Vidit!

@jywarren jywarren added the ready label May 31, 2018
@SidharthBansal
Copy link
Member Author

Yeah sure.

@jywarren
Copy link
Member

jywarren commented Jun 3, 2018

Ah, i think this was affected by the previous issue of Dockerfile config. If you rebase it over the latest master branch, it should pass!

@SidharthBansal
Copy link
Member Author

SidharthBansal commented Jun 3, 2018

@jywarren Can we ignore CodeClimate errors? I have fixed other CodeClimate errors
Thanks

@jywarren
Copy link
Member

jywarren commented Jun 3, 2018

Yes, that's fine! And if we want, we can try to look at how to tweak the CodeClimate suggestions to be less strict? Or give more people access to approve them?

I updated the settings so it'll leave a comment, too - like this: https://docs.codeclimate.com/docs/github-pull-requests#section-summary-comments

Is it helpful or not welcoming enough?

@jywarren
Copy link
Member

jywarren commented Jun 3, 2018

I think we could relax some of the CodeClimate suggestions using this guide and our config file: https://docs.codeclimate.com/docs/advanced-configuration

@jywarren jywarren merged commit c257ec8 into master Jun 3, 2018
@ghost ghost removed the ready label Jun 3, 2018
@jywarren
Copy link
Member

jywarren commented Jun 3, 2018

This looks perfect -- i hope it's all good? I merged it :-) 👍 👍 👍

@SidharthBansal
Copy link
Member Author

thanks

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* User should not be able to create Oauth based user_tags

* Added OAuth-facebook instead of facebook etc

* rerun travis

* changed the error message

* CodeClimate error fixed
@emilyashley emilyashley deleted the user_tag_can_tag branch January 15, 2020 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants