fix: Show name and emails of speakers in session notify modal #5427

divyamtayal · 2020-10-30T05:17:46Z

Fixes #5309

Short description of what this resolves:

Show Speaker Name and Speaker Email

Changes proposed in this pull request:

Added Name and Email field in speaker Notification
Added CC field (Still need to be implemented)

Checklist

I have read the Contribution & Best practices Guide.
My branch is up-to-date with the Upstream development branch.
The acceptance, integration, unit tests and linter 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)

Before

After

vercel · 2020-10-30T05:17:51Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/d2ogqrn5j
✅ Preview: https://open-event-frontend-git-speak-notif-5309.eventyay.now.sh

divyamtayal · 2020-10-30T05:21:19Z

@mariobehling @iamareebjamal Please let me know to make any changes till now and I want to know is this route implemeted

bcz I am not able to see in server documentation.
If you could tell me how to add email to request so that they can also be notified.

codecov · 2020-10-30T05:31:34Z

Codecov Report

Merging #5427 into development will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@               Coverage Diff               @@
##           development    #5427      +/-   ##
===============================================
- Coverage        23.23%   23.22%   -0.02%     
===============================================
  Files              493      493              
  Lines             5169     5172       +3     
  Branches            38       38              
===============================================
  Hits              1201     1201              
- Misses            3963     3966       +3     
  Partials             5        5

Impacted Files	Coverage Δ
app/components/modals/session-notify-modal.js	`5.55% <0.00%> (-0.51%)`	⬇️

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 e0fd0a3...9c2dbf5. Read the comment docs.

iamareebjamal · 2020-10-30T10:03:18Z

No effect without server changes

divyamtayal · 2020-10-30T10:37:48Z

No effect without server changes

So what should I do now bcz rest part is done I suppose?

mariobehling · 2020-10-30T14:32:46Z

Thank you! Please change as follows:

Create a box same a "CC" box above CC box
Show names and email addresses in the box as follows in the example: Mario Behling [email protected], Hong Phuc Dang [email protected]
Names/Emails are not editable of course

iamareebjamal · 2020-10-30T15:32:34Z

@daretobedifferent18 Create a PR on server to accept CC emails

divyamtayal · 2020-10-30T16:28:26Z

Thank you! Please change as follows:

* [ ]  Create a box same a "CC" box above CC box

* [ ]  Show names and email addresses in the box as follows in the example: Mario Behling [[email protected]](mailto:[email protected]), Hong Phuc Dang [[email protected]](mailto:[email protected])

* [ ]  Names/Emails are not editable of course

and what should be the label name for it?

divyamtayal · 2020-10-30T16:28:49Z

@daretobedifferent18 Create a PR on server to accept CC emails

@iamareebjamal So now I have to work on open-event-server also?
because if that's the case I need to start exploring the server project as well

divyamtayal · 2020-10-30T16:36:08Z

iamareebjamal · 2020-10-30T16:54:01Z

Yes this issue constitutes work in both server and frontend. As there is no point of CC box if it doesn't work. You can either work on it or wait for the server part to be completed by someone before this PR is merged. Simply, merging of the PR is blocked on server part being done

divyamtayal · 2020-10-30T16:56:43Z

Yes this issue constitutes work in both server and frontend. As there is no point of CC box if it doesn't work. You can either work on it or wait for the server part to be completed by someone before this PR is merged. Simply, merging of the PR is blocked on server part being done

Ok so meanwhile I can explore server project and for this issue frontend part is done so can I now work on other frontend issue?

iamareebjamal · 2020-10-30T16:59:11Z

You can do either of those

divyamtayal · 2020-10-30T17:01:18Z

You can do either of those

ok and do I have to make any changes now in this issue @mariobehling @iamareebjamal

app/components/modals/session-notify-modal.js

app/components/modals/session-notify-modal.hbs

iamareebjamal · 2020-10-30T17:53:40Z

app/components/modals/session-notify-modal.js

@@ -42,7 +43,7 @@ export default class SessionNotifyModal extends ModalBase {
    }
    this.mails = await mailPromise;
    const session = this.store.peekRecord('session', this.sessionId, { backgroundReload: false });
-    this.speakers = session.speakers.map(speaker => `${speaker.name} ${speaker.email}`).join(', ');
+    this.speakerEmails = session.speakers.map(speaker => `${speaker.name} ${speaker.email}`).join(', ');


See how computed properties are created

divyamtayal · 2020-10-31T06:40:18Z

@iamareebjamal Is this the correct way of declaring computed property? it is working fine with this.

iamareebjamal · 2020-10-31T14:06:56Z

app/components/modals/session-notify-modal.js

@@ -13,6 +13,12 @@ export default class SessionNotifyModal extends ModalBase {
  @tracked saving = false;
  @tracked subject = '';
  @tracked message = '';
+  @tracked cc = '';
+
+  speakerEmails =  computed(function(){


You didn't list the dependencies of the computed function

I have done it. You can please see the changes made.

…nt18/open-event-frontend into speak-notif-5309

lgtm-com · 2020-11-02T03:18:46Z

This pull request introduces 1 alert when merging 4c4bf22 into e0fd0a3 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

divyamtayal · 2020-11-02T03:34:43Z

@iamareebjamal I have added computed property for speakerEmails in session-notify-modal.js file so now you can check these changes

divyamtayal · 2020-11-02T04:21:27Z

@iamareebjamal I don't know why but on the top it is coming that the this Pr is merging from unknown repository. What should I do. The branch is there in my repo

Should I close this and make again PR?

iamareebjamal · 2020-11-02T06:15:40Z

app/components/modals/session-notify-modal.js

@@ -35,6 +36,12 @@ export default class SessionNotifyModal extends ModalBase {
    return mail;
  }

+  @computed('sessionId')
+  get speakerEmails() {
+    const session = this.store.peekRecord('session', this.sessionId, { backgroundReload: false });


Why backgroundReload: false?

Bcz I thought otherwise there will be reloading that will be visible but we can do it without specifying backgroundReload: false and can do it with default behaviour.

const session = this.store.peekRecord('session', this.sessionId);

Yes, please use this instead

divyamtayal · 2020-11-02T16:12:01Z

@iamareebjamal please let me know what to do and also about the fact that should I create new PR and close this one bcz branch is not showing up at the top.
Its showing unknown repository

iamareebjamal · 2020-11-02T18:08:21Z

No, continue in the same PR

divyamtayal · 2020-11-02T23:45:41Z

No, continue in the same PR

@iamareebjamal ok so will commiting more changes will reflect in this PR? bcz what I think is this PR is no more connected with the branch, I don't know why
Although I have contacted github support and they are saying either you have delete the current PR and make new one or they can restore but then PR made after the problem occured will be having same problem, so in that case PR that I have made after #5447 will be having same problem.
So pls let me know what to do

iamareebjamal · 2020-11-03T01:53:05Z

Just make changes using github UI then.

divyamtayal · 2020-11-03T02:22:22Z

@iamareebjamal I am not able to

Either you can do that or if there is any other option pls let me know

iamareebjamal · 2020-11-03T03:09:00Z

OK, leave the PR for now as the CC part is remaining anyway

sachinchauhan2889 · 2020-12-29T18:35:20Z

@iamareebjamal sir, you can close this. Issue has been solved

Show Email Address

8e42176

vercel bot deployed to Preview October 30, 2020 05:17 View deployment

divyamtayal changed the title ~~Show Email Address~~ Show Name and Email Address Oct 30, 2020

Show Name and Email

615f889

vercel bot deployed to Preview October 30, 2020 16:34 View deployment

iamareebjamal reviewed Oct 30, 2020

View reviewed changes

app/components/modals/session-notify-modal.js Outdated Show resolved Hide resolved

iamareebjamal reviewed Oct 30, 2020

View reviewed changes

app/components/modals/session-notify-modal.hbs Show resolved Hide resolved

iamareebjamal reviewed Oct 30, 2020

View reviewed changes

app/components/modals/session-notify-modal.hbs Outdated Show resolved Hide resolved

iamareebjamal changed the title ~~Show Name and Email Address~~ fix: Show name and emails of speakers in session notify modal Oct 30, 2020

auto-label bot added the fix label Oct 30, 2020

Code Cleanup

d97279d

vercel bot deployed to Preview October 30, 2020 17:22 View deployment

iamareebjamal reviewed Oct 30, 2020

View reviewed changes

Added Computed Prop

cf446c9

vercel bot deployed to Preview October 31, 2020 06:38 View deployment

iamareebjamal reviewed Oct 31, 2020

View reviewed changes

Merge branch 'development' into speak-notif-5309

4204a26

vercel bot deployed to Preview November 2, 2020 02:44 View deployment

daretobedifferent18 added 2 commits November 2, 2020 08:43

Added Computed Prop-1

c641fa1

Merge branch 'speak-notif-5309' of https://github.com/daretobediffere…

4c4bf22

…nt18/open-event-frontend into speak-notif-5309

vercel bot deployed to Preview November 2, 2020 03:14 View deployment

Code Cleanup-1

5eda544

vercel bot deployed to Preview November 2, 2020 03:21 View deployment

Code Cleanup-2

2202962

vercel bot deployed to Preview November 2, 2020 03:24 View deployment

Code Cleanup-3

82463aa

vercel bot deployed to Preview November 2, 2020 03:30 View deployment

Code Cleanup-4

9c2dbf5

vercel bot deployed to Preview November 2, 2020 03:41 View deployment

iamareebjamal reviewed Nov 2, 2020

View reviewed changes

iamareebjamal closed this Dec 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Show name and emails of speakers in session notify modal #5427

fix: Show name and emails of speakers in session notify modal #5427

divyamtayal commented Oct 30, 2020 •

edited

Loading

vercel bot commented Oct 30, 2020 •

edited

Loading

divyamtayal commented Oct 30, 2020

codecov bot commented Oct 30, 2020 •

edited

Loading

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

mariobehling commented Oct 30, 2020

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020 •

edited

Loading

divyamtayal commented Oct 30, 2020

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

iamareebjamal Oct 30, 2020

divyamtayal commented Oct 31, 2020

iamareebjamal Oct 31, 2020

divyamtayal Nov 2, 2020

lgtm-com bot commented Nov 2, 2020

divyamtayal commented Nov 2, 2020 •

edited

Loading

divyamtayal commented Nov 2, 2020 •

edited

Loading

iamareebjamal Nov 2, 2020

divyamtayal Nov 2, 2020 •

edited

Loading

iamareebjamal Nov 2, 2020

divyamtayal commented Nov 2, 2020

iamareebjamal commented Nov 2, 2020

divyamtayal commented Nov 2, 2020 •

edited

Loading

iamareebjamal commented Nov 3, 2020

divyamtayal commented Nov 3, 2020 •

edited

Loading

iamareebjamal commented Nov 3, 2020

sachinchauhan2889 commented Dec 29, 2020

fix: Show name and emails of speakers in session notify modal #5427

fix: Show name and emails of speakers in session notify modal #5427

Conversation

divyamtayal commented Oct 30, 2020 • edited Loading

Short description of what this resolves:

Changes proposed in this pull request:

Checklist

vercel bot commented Oct 30, 2020 • edited Loading

divyamtayal commented Oct 30, 2020

codecov bot commented Oct 30, 2020 • edited Loading

Codecov Report

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

mariobehling commented Oct 30, 2020

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020 • edited Loading

divyamtayal commented Oct 30, 2020

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

iamareebjamal commented Oct 30, 2020

divyamtayal commented Oct 30, 2020

iamareebjamal Oct 30, 2020

Choose a reason for hiding this comment

divyamtayal commented Oct 31, 2020

iamareebjamal Oct 31, 2020

Choose a reason for hiding this comment

divyamtayal Nov 2, 2020

Choose a reason for hiding this comment

lgtm-com bot commented Nov 2, 2020

divyamtayal commented Nov 2, 2020 • edited Loading

divyamtayal commented Nov 2, 2020 • edited Loading

iamareebjamal Nov 2, 2020

Choose a reason for hiding this comment

divyamtayal Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

iamareebjamal Nov 2, 2020

Choose a reason for hiding this comment

divyamtayal commented Nov 2, 2020

iamareebjamal commented Nov 2, 2020

divyamtayal commented Nov 2, 2020 • edited Loading

iamareebjamal commented Nov 3, 2020

divyamtayal commented Nov 3, 2020 • edited Loading

iamareebjamal commented Nov 3, 2020

sachinchauhan2889 commented Dec 29, 2020

divyamtayal commented Oct 30, 2020 •

edited

Loading

vercel bot commented Oct 30, 2020 •

edited

Loading

codecov bot commented Oct 30, 2020 •

edited

Loading

divyamtayal commented Oct 30, 2020 •

edited

Loading

divyamtayal commented Nov 2, 2020 •

edited

Loading

divyamtayal commented Nov 2, 2020 •

edited

Loading

divyamtayal Nov 2, 2020 •

edited

Loading

divyamtayal commented Nov 2, 2020 •

edited

Loading

divyamtayal commented Nov 3, 2020 •

edited

Loading