-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: Allow organizer to add speaker without email #5359
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/cp7kkkfkq |
Codecov Report
@@ Coverage Diff @@
## development #5359 +/- ##
============================================
Coverage 22.78% 22.78%
============================================
Files 491 491
Lines 5245 5245
Branches 37 37
============================================
Hits 1195 1195
Misses 4045 4045
Partials 5 5
Continue to review full report at Codecov.
|
@@ -11,7 +11,7 @@ export default class extends Controller { | |||
} | |||
const newSpeaker = this.model.speaker; | |||
if (newSpeaker.isEmailOverridden) { | |||
newSpeaker.set('email', this.authManager.currentUser.email); | |||
newSpeaker.set('email', 'Not Provided'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that is wrong data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what should be put here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I am setting it to null it throws err - the server responded with a status of 422 ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Server needs to allow null emails in case organizers or admin create the speaker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after allowing null from server and adding null here, it is not being null, It sets to user email
on writing newSpeaker.set('email', null); .
But on writingnewSpeaker.set('email', ' any text');
the email is set to that text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is happening because of this on server side :
elif (
data.get('is_email_overridden')
and has_access('is_organizer', event_id=data['event'])
and not data.get('email')
):
data['email'] = current_user.email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will check why this code was added.
@maze-runnar One condition is still left. If a non-organizer user sets email to null, it should be denied |
Here is an overview of what got changed by this pull request: Complexity increasing per file
==============================
- app/controllers/events/view/speakers/create.js 5
See the complete overview on Codacy |
Fixes #5342
Checklist
development
branch.