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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display user email for administrators #1808

Merged
merged 5 commits into from Oct 7, 2019

Conversation

aonomike
Copy link
Member

@aonomike aonomike commented Oct 3, 2019

馃嵃 Pullrequest

  • Display user email to administrators
  • Refactor Permission middleware tests
  • Add tests to check email of user only revealed to admin and owner

Issues

fixes #1704

Images

Screen Shot 2019-10-05 at 5 29 31 PM

@@ -128,6 +138,7 @@ export default {
id
name
slug
email
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, the backend does not allow anybody except the owner to see the email of the user. You will need to change permissions in the backend.

I think I would still feel safe, if only the owner or an admin user can see the email.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks @roschaefer

@aonomike aonomike changed the title [WIP] Display user email for administrators Display user email for administrators Oct 5, 2019
Copy link
Member

@mattwr18 mattwr18 left a comment

Choose a reason for hiding this comment

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

Looks good to me @aonomike!
And you even refactored another backend test!!
Great job!!

Copy link
Contributor

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

hesitant

To me, this looks good and I can't see why we should not merge it. It's tested and there is nothing that comes to my mind, which we need to do before merging. Great job @aonomike !

But I'm a little hesitant to merge this before our daily standup. We could hear back from the others if we missed anything important here. it's a matter of security/data-privacy after all 馃槈

).resolves.toMatchObject({
errors: [{ message: 'Not Authorised!' }],
data: { User: [null] },
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so much better 馃槏

@@ -33,6 +33,11 @@
<b>{{ scope.row.name | truncate(20) }}</b>
</nuxt-link>
</template>
<template slot="email" slot-scope="scope">
<a :href="`mailto:${scope.row.email}`">
<b>{{ scope.row.email }}</b>
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of those emails are quite long. Maybe we should think about how to fit the data into the layout? E.g. at some places in the app, we use | truncate(20) filter.

Although I'm OK to merge this and wait for our admins to complain about it 馃槈

Copy link
Member

Choose a reason for hiding this comment

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

That's what I thought as well, @datenbrei mentioned that they might be copying and pasting the links, so maybe it's more difficult if it's truncated??

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @mattwr18 @roschaefer
Let me know if I need to update this :D

@mattwr18 mattwr18 merged commit 23841b9 into master Oct 7, 2019
@mattwr18 mattwr18 deleted the 1704_display-user-email-to-moderators branch October 7, 2019 10:33
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.

馃殌 [Feature] Make Email of a User visible for Moderators
3 participants