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

feat: add field for email exception in speaker schema #6209

Merged
merged 1 commit into from Jul 19, 2019

Conversation

shreyanshdwivedi
Copy link
Member

Fixes #6059

Short description of what this resolves:

When a speaker registers they have to enter an email. It must be possible to override this requirement for organizers/admins, e.g. if a VIP speaker does not want to sign up himself

Changes proposed in this pull request:

  • add field and migration
  • mute email and notifications for overridden speakers

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #6209 into development will decrease coverage by 0.03%.
The diff coverage is 22.22%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6209      +/-   ##
===============================================
- Coverage        65.97%   65.93%   -0.04%     
===============================================
  Files              288      288              
  Lines            14538    14552      +14     
===============================================
+ Hits              9591     9595       +4     
- Misses            4947     4957      +10
Impacted Files Coverage Δ
app/api/sessions.py 41.32% <0%> (-0.35%) ⬇️
app/api/helpers/scheduled_jobs.py 21.42% <0%> (-0.18%) ⬇️
app/models/speaker.py 94.52% <100%> (+0.15%) ⬆️
app/api/schema/speakers.py 100% <100%> (ø) ⬆️
app/api/speakers.py 47.87% <11.11%> (-3.9%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d6399d...e11fbac. Read the comment docs.

app/api/sessions.py Outdated Show resolved Hide resolved
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

Won't you need to make changes in schema too, As email is required field for a speaker or are we going to use organiser email?

@shreyanshdwivedi
Copy link
Member Author

We are going to use organizer's email. @kushthedude

kushthedude
kushthedude previously approved these changes Jul 18, 2019
@shreyanshdwivedi
Copy link
Member Author

@kushthedude @mrsaicharan1 @uds5501 this PR is up for review.
@kushthedude can you please try out your FE changes on local with these changes and check if it is working fine and raising an error perfectly or not.

@fossasia fossasia deleted a comment Jul 19, 2019
@fossasia fossasia deleted a comment Jul 19, 2019
app/api/speakers.py Outdated Show resolved Hide resolved
@fossasia fossasia deleted a comment Jul 19, 2019
raise ForbiddenException({'pointer': 'data/attributes/is_email_overriden'},
'Co-organizer access required to override email')
elif has_access('is_coorganizer', event_id=data['event']) and data.get('is_email_overriden'):
data['email'] = current_user.email
Copy link
Member

Choose a reason for hiding this comment

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

If email is overridden, then when is this email used?

Copy link
Member Author

Choose a reason for hiding this comment

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

If email is overriden then organizer's email is used. I'm adding this line so that we don't depend solely on client to send email.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but in above functions, you don't send email if it is overriden. Reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I misread your first question. This email acts as a dummy to satisfy the schema constraints (as email cannot be null) and can have secondary use to let us know which co-organizer added him/her.
And we are not sending email because it will act as a spam to organizer since he is already getting every notif and email.

Copy link
Member

Choose a reason for hiding this comment

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

Then only change data['email'] if it is not present. Or else, it is wrong info. Also, if someone disables email override, you will now definitely spam the organizer

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the logic and updated data['email'] only if it isn't present

@fossasia fossasia deleted a comment Jul 19, 2019
mute notifications and mails for overriden speakers

adds checks

fixes travis

removes redundant None in data.get

update permission to organizer access

update email logic
@iamareebjamal iamareebjamal merged commit 5802c42 into fossasia:development Jul 19, 2019
iamareebjamal pushed a commit to iamareebjamal/open-event-server that referenced this pull request Aug 2, 2019
@shreyanshdwivedi shreyanshdwivedi deleted the emailOverride branch August 3, 2019 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addition of Field for Override Email for Speaker
5 participants