Option to enable and validate Payment Phone Numbers #428
Conversation
| @property | ||
| def object_list(self): | ||
| # Override this | ||
| return [] | ||
|
|
||
| @property | ||
| def report_url(self): |
There was a problem hiding this comment.
all these functions that must be overriden should raise a NotImplementedError so that failure to do so results in a hard failure not silently wrong behavior.
There was a problem hiding this comment.
Good point, will update
commcare_connect/reports/views.py
Outdated
| # Inherit this for a tabular report | ||
| page_template = "reports/report_table.html" | ||
| htmx_table_template = "reports/htmx_table.html" | ||
| report_title = "Override this" |
There was a problem hiding this comment.
Is there a way to force people to set this, rather than make it possible to have a functioning report with this base value
There was a problem hiding this comment.
If we make it a NonImplemented property method we could do that, but that might not be super necessary. GenericReportView in HQ sets these to just None. As one implements a report one would look at the report title and hopefully change it. I made this catchy 'Override this' instead of None.
There was a problem hiding this comment.
Why is that better than None? None seems pretty clear and clean, and is nice convention to have, rather than a random value, even one that would be obvious on second look.
| return queryset | ||
|
|
||
|
|
||
| class PaymentNumberReport(tables.SingleTableMixin, SuperUserRequiredMixin, NonModelTableBaseView): |
There was a problem hiding this comment.
If only superusers can do this, does that mean that every project will need to tell someone at dimagi which numbers work, and have a dimagi person update it? That doesn't seem like the desired workflow for network manager run opportunities, and seems unsustainable as well.
There was a problem hiding this comment.
I was planning to update the role here and forgot. I will change the permissions to org-user (can manage opportunities)
There was a problem hiding this comment.
Can you explain a little more about the expected workflows and security model? It seems like if a user is in a multiple different opportunities across orgs, any member of any of those orgs can impact their ability to get paid by the other org by changing this value.
There was a problem hiding this comment.
If a user is able to create/edit an opportunity, the user should be able to manage the payment numbers. It looks like I need to change to use OrganizationUserMemberRoleMixin (which is also used in OpportunityEdit) instead of OrganizationUserMixin. (I have updated)
The payment features can be turned on any opportunity (whether managed or not).
If we are expecting a managed property to be manageable only by specific users and not other users who can edit any opportunity, then the OrganizationUserMemberRoleMixin is not enough. But it looks like this is how we are doing permissioning for opportunity edit view as well, so this is consistent with what we have.
There was a problem hiding this comment.
My concern is that they are setting a global property for the user that affects other opportunities as well. I think edit opportunity/org member permissions are reasonable for changes that are isolated to that opportunity, but here the actions they take affect the other opportunities the user is a part of, and their ability to deliver or be paid in those opportunities as well.
There was a problem hiding this comment.
True. Payment profiles are not org/opportunity specific, but any org with payment-info-required opportunities will be able to manage them.
I believe we discussed this earlier in the weekly calls (during the spec process) and we were fine with an org being able to say that a payment number can or can not receive payments. (Update: I have double checked with Rama again on this.)
This tool makes it necessary to select an opportunity (only from those that have payments enabled) and then fetches users' payment profiles for that opportunity only for managing.
There was a problem hiding this comment.
we were fine with an org being able to say that a payment number can or can not receive payments.
I recall being fine with orgs blocking payment for their opportunity, I am very concerned about their ability to block it for other opportunities, especially ones they are active in, and potentially already getting paid. Happy to take this offline, but I do not believe it is reasonable behavior to have different orgs be able to override each other's work here, and I think this approval should probably be org or opportunity specific. That would also address the concern previously raised about these numbers no longer working over time, since they would be approved each time.
|
Hey @calellowitz I have updated this to let orgs maintain their own status of whether a payment number works or not as discussed on slack. I might restructure the tests to use factory instead of the way I did mocks. |
| @@ -83,7 +92,17 @@ def object_list(self): | |||
| usernames = OpportunityAccess.objects.filter(opportunity_id=opportunity).values_list( | |||
| "user__username", flat=True | |||
| ) | |||
| return fetch_payment_phone_numbers(usernames, status) | |||
| connecteid_statuses = fetch_payment_phone_numbers(usernames, status) | |||
There was a problem hiding this comment.
nit: typo in variable name
sravfeyn
force-pushed
the
sr/payments
branch
from
cf896eb to
60e9fcb
Compare
sravfeyn
force-pushed
the
sr/payments
branch
from
3d364cc to
363b3e2
Compare
Spec
Option to enable
UI to validate numbers
