mitodl / micromasters

Portal for learners and course teams to access MITx Micromasters® programs
https://mm.mit.edu
BSD 3-Clause "New" or "Revised" License
30 stars 17 forks source link

Enable learner-to-learner emails with upsell message #2888

Open gsidebo opened 7 years ago

gsidebo commented 7 years ago

Depends on #2512

2196 was initially spec'ed out to fully allow learners to email other learners from a recipient learner's profile page, and to show an upsell message if an unpaid learner was attempting to message another learner. The requirements were trimmed due to the fact that #2512 is still in progress and those features depend on that work being done. #2196 was instead completed in a way that allows staff users to email learners from the learner's profile page, and hides the link for non-staff users.

Note on payment conditions: Learner A should be able to message Learner B if Learner A has paid for any course run in any program that both learners are enrolled in. mail.permissions.UserCanMessageSpecificLearnerPermission already has this logic built in, so any requests from unpaid learners should already be denied, but we will still need to make that payment/non-payment status available in the front-end so that a decision can be made about whether we show a learner the email dialog or the upsell message (as we do with the course team contact link).

Acceptance criteria:

amir-qayyum-khan commented 7 years ago

@pdpinch @noisecapella @aliceriot

1) should i move this to app.js so that profile, and search components can invoke it.

2) On profile/learner page, we have dashboard object, but how can i identify that the current user has paid in course or not? Should i allow him to send email if he has paid for any course? Because user can have multiple courses and he can be paid in some and unpaid in others.

3) On /learners page we dont have dashboard object, we dont know if user is paid or not, should i add dashboard api to learners search?

pdpinch commented 7 years ago

@amir-qayyum-khan maybe you should put this on hold until @gsidebo is back in the office. I was under the impression that we had already addressed these issues, but perhaps not.

amir-qayyum-khan commented 7 years ago

ok

amir-qayyum-khan commented 7 years ago

@gsidebo @pdpinch

1) should i move this to app.js so that profile, and search components can invoke it.

2) What is the criteria of allowing user to send email to other learners i.e enrolled, paid or any other?

3) On /learners page we dont logged in user payment info, should i get this info from dashboard api to learners search?

gsidebo commented 7 years ago

Hey @amir-qayyum-khan

  1. Did you link to the correct line in this question? Your link points to setShowExpandedCourseStatus. I guess I would need to know why that function needs to be invoked in places that it cannot currently be invoked

  2. Criteria are in the issue description:

    Learner A should be able to message Learner B if Learner A has paid for any course run in any program that both learners are enrolled in

  3. I think it's probably best not to call the dashboard API on this page. Retrieving search results takes a decent amount of time as it is. Calling the full dashboard API in addition to fetching search results could be really slow. I think we might want to add a new lightweight API endpoint that acts as a dashboard 'summary' (i.e.: it fetches a limited amount of dashboard data that we care about in this context). In this case, we just need to know the programs in which a user has made a payment, e.g.: {'payments': {'program_1': true, 'program_2': false}}. We could also somehow combine this information with the profile API to cut down on the number of HTTP calls, but that seems a little messier. @noisecapella @aliceriot, thoughts on this? I haven't worked on MM for a little while.

amir-qayyum-khan commented 7 years ago

Did you link to the correct line in this question? Your link points to setShowExpandedCourseStatus. I guess I would need to know why that function needs to be invoked in places that it cannot currently be invoked

https://github.com/mitodl/micromasters/blob/master/static/js/containers/DashboardPage.js#L568

@gsidebo updated link

amir-qayyum-khan commented 7 years ago

I think it's probably best not to call the dashboard API on this page. Retrieving search results takes a decent amount of time as it is. Calling the full dashboard API in addition to fetching search results could be really slow. I think we might want to add a new lightweight API endpoint that acts as a dashboard 'summary' (i.e.: it fetches a limited amount of dashboard data that we care about in this context). In this case, we just need to know the programs in which a user has made a payment, e.g.: {'payments': {'program_1': true, 'program_2': false}}. We could also somehow combine this information with the profile API to cut down on the number of HTTP calls, but that seems a little messier. @noisecapella @aliceriot, thoughts on this? I haven't worked on MM for a little while.

@gsidebo I think we need an api that give us status that if user is allowed leaner to learner search. We can ignore dashboard and prices? what are you thoughts? In that api we can check if he is enroll in atleast one course and has paid, we can change this logic anytime from backend without change front end.

Api should return that user has permission {True, False}

alicewriteswrongs commented 7 years ago

Could you put that information in SETTINGS.roles, like we do for the staff roles?

gsidebo commented 7 years ago

As I understand, it's not really a question of roles. Every user has permission to email another user if they have paid for any course run in any program that the target user is enrolled in. Do we actually have a role/permission that says which learners can use the search? I'm not aware of anything like that. I assumed that any learner could search within their enrolled programs.

I think the easiest option here that doesn't kill performance is to create the new API endpoint. I suggested above that the endpoint could return the payment status for all programs ({'payments': {'program_1': true, 'program_2': false}}). That would be useful when viewing a profile page directly. The profile page fetches data from the dashboard API for the user you're viewing. You can match the program enrollment info in that user's dashboard data with the program payment data for the logged-in user and determine if the logged in user is able to send the message. For the search results page, you only need to find out if the logged in user has paid for anything in the currently selected program.

gsidebo commented 7 years ago

@amir-qayyum-khan Answering your other question about renderCourseContactPaymentDialog...

I think the best thing to do would be to create a new component called PaymentTeaserDialog (or something like that) and render that component in DashboardPage#renderCourseContactPaymentDialog. The new component would specify the props that are the same between the payment teaser dialogs, and accept props for things like the title, UI state, body text, etc.

I also think this dialog should use the generalized showDialog and hideDialog actions (static/js/actions/ui.js:10), and the current course team contact dialog should be refactored to use that. We had been creating sets of actions/reducers for showing and hiding every single type of dialog up until we started using these, and that requires a lot of boilerplate. To use showDialog/hideDialog with the payment teaser dialogs, you would need to define a constant for the two types of payment dialogs (COURSE_CONTACT_TEASER_DIALOG and LEARNER_EMAIL_TEASER_DIALOG, or something like that), and pass that into showDialog/hideDialog as needed. If we created a PaymentTeaserDialog component, you could accept those constants as a prop called dialogName or something like that, and the component would know all it needs to know to show or hide the dialog. Example:

<PaymentTeaserDialog
    title='Send a message to another learner'
    dialogState=ui.dialogVisibility
    dialogName=LEARNER_EMAIL_TEASER_DIALOG
    ...
/>

Let me know if any of that is confusing or if you have any different ideas