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

Dont show emails or real names #2008

Closed

Conversation

captn3m0
Copy link
Contributor

@captn3m0 captn3m0 commented Mar 18, 2018

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the upstream master branch.
  • The tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works(if appropriate).
  • I have added necessary documentation (if appropriate).

Short description of what this resolves/which issues does this fix?:

Changes proposed in this pull request:

  • Removes real names and email addresses from speaker select lists
  • Trims down the list to just participants in the current conference

Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

@captn3m0 thanks a lot for the PR! 💐

You need to squash your "Fixes style issues" commit in the first one. It is not nice to have a commit correction "errors" introduced in a previous commit. 😉

Apart from that the code LGTM. As I commented in the issue, I do not see the reason why we want to remove the name. 😕

@captn3m0
Copy link
Contributor Author

@Ana06 Thanks for the quick review. I decided to stick only with usernames because the profile page calls out the username field as "how others users see you". I'll change it to "Name (username)".

There are some test failures that I missed. Will fix them soon.

PS: This is live at https://osem.hillhacks.in

@Ana06
Copy link
Member

Ana06 commented Mar 21, 2018

@captn3m0

@Ana06 Thanks for the quick review. I decided to stick only with usernames because the profile page calls out the username field as "how others users see you". I'll change it to "Name (username)".

There are some test failures that I missed. Will fix them soon.

great! 😉

PS: This is live at https://osem.hillhacks.in

cool! 🎉 maybe you want to add it to https://github.com/openSUSE/osem/wiki/Conferences-using-OSEM 😉

@captn3m0 captn3m0 force-pushed the dont-show-emails-or-real-names branch from a32da2b to 17614ce Compare March 21, 2018 19:33
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

@captn3m0 thanks a lot! The code looks great! 💘

I think the Travis failures are flickering tests, but I have just restarted... let see if it passes now...

@captn3m0
Copy link
Contributor Author

@Ana06 Test failures are because the speaker_ids field doesn't get auto-populated anymore on the proposal edit page.

I tried with select(organizer.id, from: 'event[speaker_ids][]') to select it, but couldn't get it working.

@Ana06
Copy link
Member

Ana06 commented Apr 16, 2018

@captn3m0

the speaker_ids field doesn't get auto-populated anymore on the proposal edit page.

What do you exactly mean? 🤔

@captn3m0
Copy link
Contributor Author

captn3m0 commented Apr 16, 2018

This is how the proposal edit page looks now:

image

And this is how it looks on master:

image

Tested a bit more, realized it is because since the new code only shows: "registered users for this conference" in the potential speaker list, it skips out on the current user as well since registration is not part of the test.

I can either:

  1. Force register the user for the test, but this might make the proposal edit page frustrating for users since you don't see your name in the speaker list till you register (which you might miss)
  2. Add current user as an exception to the speaker list always.

- Fixes openSUSE#1884
- Decided to remove real names as well, because the profile page
  calls out the username field as "how others users see you"
- Instead of showing Users from all the events, only use
  the current conference participants instead
@captn3m0 captn3m0 force-pushed the dont-show-emails-or-real-names branch from 17614ce to 6bf3562 Compare April 16, 2018 18:51
@captn3m0
Copy link
Contributor Author

@Ana06 Any help on the above?

@bear454 bear454 mentioned this pull request Jun 15, 2018
3 tasks
@bear454 bear454 self-requested a review June 20, 2018 21:19
Copy link
Contributor

@bear454 bear454 left a comment

Choose a reason for hiding this comment

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

Email inclusion should be based on the value of User#email_public.

@hennevogel
Copy link
Member

Sorry but I think I close this now, this seems very stale. Feel free to send the same changes again if you're ready to work on it! Thank you anyway 💐

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.

Personal information leak when editing proposals
4 participants