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

Models and signup & login views for consumer_user_poc #155

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

challabeehyv
Copy link
Owner

Summary

Feature Flag

Product Description

Safety Assurance

  • Risk label is set correctly
  • All migrations are backwards compatible and won't block deploy
  • The set of people pinged as reviewers is appropriate for the level of risk of the change
  • If QA is part of the safety story, the "Awaiting QA" label is used
  • I am certain that this PR will not introduce a regression for the reasons below

Automated test coverage

QA Plan

Safety story

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Copy link

@proteusvacuum proteusvacuum left a comment

Choose a reason for hiding this comment

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

Great start!

from django.urls import reverse


def login_required(view_func):

Choose a reason for hiding this comment

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

this seems to be copied from https://github.com/dimagi/commcare-hq/blob/master/corehq/apps/domain/decorators.py#L517-L526
Is there a reason you need a new decorator?

Choose a reason for hiding this comment

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

Bump.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Created a new decorator so that we could redirect to consumer_user login if the current user logged in user doesn't have corresponding ConsumerUser instance. ConsumerUserCaseRelationship has relation ship to ConsumerUser. Once he logins corresponding ConsumerUser will be created.

consumer_user = ConsumerUser.objects.create(user=user)
consumer_user.save()
if obj:
obj.accepted = True

Choose a reason for hiding this comment

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

note so I don't forget: we'll need to update the commcare-caseuserinvitation that created this invitation in the first place here.

@proteusvacuum
Copy link

When you are ready for a more thorough review (i.e. when more of the spec is complete), please make this as a PR against the upstream repo so our tests and other checks will be run against it.

To ensure you don't have a lot of fomatting and lint changes need to be made, it would be good to ensure you run flake8 (with our settings) against your changes and install the git hooks

Copy link

@proteusvacuum proteusvacuum left a comment

Choose a reason for hiding this comment

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

I did a first pass, but will let you clean things up before going through it again.

@@ -519,7 +519,8 @@ def login_required(view_func):
@wraps(view_func)
def _inner(request, *args, **kwargs):
user = request.user
if not (user.is_authenticated and user.is_active):
couch_user = CouchUser.get_by_username(user.username)

Choose a reason for hiding this comment

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

could maybe put this in the middleware as a new variable on request?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Currently redirecting to login page if user does not have corresponding CouchUser. Once the user logins couchuser will be created set on the request object. From subsequent calls as couch_user will be present it wont redirect to login

self.save(update_fields=['accepted'])

@classmethod
def create_invitation(cls, case_id, domain, opened_by, email):

Choose a reason for hiding this comment

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

nit: what's the purpose of this method? In the place you call this, you could just replace this call with ConsumerUserInviation.objects.create(...) right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Created this before when code was clumsy

if invitation:
make_invitation_inactive(invitation, case)
return
elif invitation and email == invitation.email:

Choose a reason for hiding this comment

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

Is there a way we can resend an invitation in this case?

Copy link
Owner Author

@challabeehyv challabeehyv Mar 2, 2021

Choose a reason for hiding this comment

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

Made new changes to signals logic. If we set the status in case_properties to anything other than 'sent' or 'accepted' it should create a new invitation and send a new mail.

update_case(case.domain, case.case_id, {'invitation_status': 'sent', 'updated_by': 'patient'})
def make_invitation_inactive(invitation, case):
invitation.make_inactive()
ConsumerUserCaseRelationship.objects.filter(case_id=case.case_id,

Choose a reason for hiding this comment

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

I still don't think we want this to happen. We should instead block a new invitation being made in this instance.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Made some changes that would block creating new invitation if there is a new ConsumerUserCaseRelationship exists



def send_email_case_changed_receiver(sender, case, **kwargs):
if case.type != CONSUMER_INVITATION_CASE_TYPE:
Copy link

@proteusvacuum proteusvacuum Mar 1, 2021

Choose a reason for hiding this comment

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

I think you need to add an extra check here

case.get_case_property(CONSUMER_INVITATION_STATUS) == CONSUMER_INVITATION_SENT

Otherwise you'll end up with an infinite loop.

Once you fix that, you'll see there is still a failing test.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Made the change inside the method. If we keep it at top when the case is closed it wont update the corresponding invitation

@task
def create_new_invitation(case_id, domain, opened_by, email):
invitation = ConsumerUserInvitation.create_invitation(case_id, domain, opened_by, email)
url = '%s%s' % (get_url_base(),

Choose a reason for hiding this comment

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

FYI I get this error when running this in celery:

2021-03-02 21:23:54,886: INFO/MainProcess] Received task: corehq.apps.consumer_user.tasks.create_new_invitation[1d1e586c-7ef7-498d-acd6-61372c937621]  
[2021-03-02 21:23:54,920: ERROR/ForkPoolWorker-6] Task corehq.apps.consumer_user.tasks.create_new_invitation[1d1e586c-7ef7-498d-acd6-61372c937621] raised unexpected: NoReverseMatch("'consumer_user' is not a registered namespace")
Traceback (most recent call last):
  File "/home/frener/.virtualenvs/cchq/lib/python3.8/site-packages/django/urls/base.py", line 75, in reverse
    extra, resolver = resolver.namespace_dict[ns]
KeyError: 'consumer_user'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/frener/.virtualenvs/cchq/lib/python3.8/site-packages/celery/app/trace.py", line 375, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/home/frener/.virtualenvs/cchq/lib/python3.8/site-packages/celery/app/trace.py", line 632, in __protected_call__
    return self.run(*args, **kwargs)
  File "/home/frener/dev/dimagi/commcare-hq/corehq/apps/consumer_user/tasks.py", line 24, in create_new_invitation
    reverse('consumer_user:consumer_user_register',
  File "/home/frener/.virtualenvs/cchq/lib/python3.8/site-packages/django/urls/base.py", line 86, in reverse
    raise NoReverseMatch("%s is not a registered namespace" % key)
django.urls.exceptions.NoReverseMatch: 'consumer_user' is not a registered namespace

Copy link
Owner Author

Choose a reason for hiding this comment

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

Its working in my local system when I trigger the call directly.

[2021-03-03 10:19:33,850: INFO/MainProcess] Received task: corehq.apps.consumer_user.tasks.create_new_invitation[1138ed38-8487-41f9-8df2-5ff10bdd453c]  
[2021-03-03 10:19:34,204: WARNING/ForkPoolWorker-6] http://localhost:8000/hq/consumer_user/signup/MjQ:1lHObN:-8hAS0-6c2CSKgckOVNlP60X5Z4/
[2021-03-03 10:19:34,248: INFO/ForkPoolWorker-6] Received Form a61e5f87f4a94f04ac0825ea3636fdae with 0 attachments
[2021-03-03 10:19:34,310: INFO/ForkPoolWorker-6] Finished normal processing for Form a61e5f87f4a94f04ac0825ea3636fdae
[2021-03-03 10:19:34,313: INFO/ForkPoolWorker-6] Task corehq.apps.consumer_user.tasks.create_new_invitation[1138ed38-8487-41f9-8df2-5ff10bdd453c] succeeded in 0.4627520860012737s: None
[2021-03-03 10:20:08,799: INFO/MainProcess] Received task: corehq.apps.consumer_user.tasks.create_new_invitation[e5fd1b81-a816-4b1f-96b0-13cd446b1678]  
[2021-03-03 10:20:09,169: WARNING/ForkPoolWorker-12] http://localhost:8000/hq/consumer_user/signup/MjU:1lHObw:voZOGBr9qQtJTYLfyIRFPf_DpUY/
[2021-03-03 10:20:09,203: INFO/ForkPoolWorker-12] Received Form eb1e4a8de3c54fbfb79780cc4bcaf3ae with 0 attachments
[2021-03-03 10:20:09,272: INFO/ForkPoolWorker-12] Finished normal processing for Form eb1e4a8de3c54fbfb79780cc4bcaf3ae
[2021-03-03 10:20:09,276: INFO/ForkPoolWorker-12] Task corehq.apps.consumer_user.tasks.create_new_invitation[e5fd1b81-a816-4b1f-96b0-13cd446b1678] succeeded in 0.4763715970002522s: None
[2021-03-03 10:20:15,488: INFO/MainProcess] Received task: corehq.apps.consumer_user.tasks.create_new_invitation[1edfe58b-76c2-431c-b6a8-22f52cbea709]  
[2021-03-03 10:20:15,863: WARNING/ForkPoolWorker-1] http://localhost:8000/hq/consumer_user/signup/MjY:1lHOc3:fodmjQtbPEgqF-8lyRJpa3NZ_04/
[2021-03-03 10:20:15,893: INFO/ForkPoolWorker-1] Received Form 5ebe229fde2e4ea68c22f4529541d82a with 0 attachments
[2021-03-03 10:20:15,951: INFO/ForkPoolWorker-1] Finished normal processing for Form 5ebe229fde2e4ea68c22f4529541d82a
[2021-03-03 10:20:15,955: INFO/ForkPoolWorker-1] Task corehq.apps.consumer_user.tasks.create_new_invitation[1edfe58b-76c2-431c-b6a8-22f52cbea709] succeeded in 0.466650219001167s: None
[2021-03-03 10:20:16,090: INFO/MainProcess] Received task: corehq.apps.consumer_user.tasks.create_new_invitation[33e3fcae-5e4a-4d3a-91b3-7a922718d73d]  
[2021-03-03 10:20:16,471: WARNING/ForkPoolWorker-2] http://localhost:8000/hq/consumer_user/signup/Mjc:1lHOc4:hVJ8QRhq3jWPmfuHlxua-1scLos/
[2021-03-03 10:20:16,504: INFO/ForkPoolWorker-2] Received Form cb44518343e446f39fcaa08b806da419 with 0 attachments
[2021-03-03 10:20:16,561: INFO/ForkPoolWorker-2] Finished normal processing for Form cb44518343e446f39fcaa08b806da419
[2021-03-03 10:20:16,565: INFO/ForkPoolWorker-2] Task corehq.apps.consumer_user.tasks.create_new_invitation[33e3fcae-5e4a-4d3a-91b3-7a922718d73d] succeeded in 0.4747796890005702s: None

Copy link
Owner Author

Choose a reason for hiding this comment

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

I checked if I missed anything while pushing. I pushed all the changes I made. Can you tell me how are you triggering this task? I will try to replicate in my system

Choose a reason for hiding this comment

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

Weird, I was testing by creating a case through an app I built locally. I will try again and see if there is anything with my local setup that was causing this.

return
email = case.get_case_property('email')
status = case.get_case_property(CONSUMER_INVITATION_STATUS)
invitation = ConsumerUserInvitation.objects.filter(case_id=case.case_id,

Choose a reason for hiding this comment

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

I'm not sure how I didn't catch this before, but this is the wrong case_id. From the spec, this should be the "uuid for the patient/demographic case."
It would be something like:

host_indices = [index for index in case.indices where index.relationship = 'extension']
try:
    person_case_id = host_indices[0].referenced_id
except IndexError:
    return    # the app is set up incorrectly.

Copy link
Owner Author

@challabeehyv challabeehyv Mar 3, 2021

Choose a reason for hiding this comment

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

We only save case_id, domain in both ConsumerUserInvitation and ConsumerUserCaseRelationship models as mentioned in the document. You want me to store uuid for the patient/demographic case in the case_id right. I tried implementing the above logic but getting host_indices as empty list when I call from tests using create_case. I tried looking at other usages but could n't find anything helpful. Can you share here how to use this correctly?

Also, Since we are now planning to use uuid for the patient/demographic case in the case_id, will update_case work with this reference_id? Should I add a new column in both ConsumerUserInvitation and ConsumerUserCaseRelationship models to store case_id too?

Choose a reason for hiding this comment

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

You can create indexed cases like this:

https://github.com/challabeehyv/commcare-hq/blob/consumer_user_poc/corehq/ex-submodules/casexml/apps/case/tests/test_db_accessors.py#L178-L186

So you would need to make both a demographic, and an invitation case in your tests.

We want the CaseRelationship to link to the demographic case, and we would want to store that id on the invitation when it is created to stop a case getting reassigned between the invitation being created and being accepted.

I think if there can only be a single invitation case open for a particular demographic case, we should be able to always get the invitation back (by looking at the reverse indexed cases for one with the invitation type).

However, I agree that it might be better to store both invitation case_id and the demographic case_id on the ConsumerUserInvitation. I don't think you need to add this to the ConsumerUserCaseRelationship.

Copy link

@proteusvacuum proteusvacuum left a comment

Choose a reason for hiding this comment

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

Do you think this is in a good place for you to open this PR against the upstream repo? I'd like our full test suite to run against it, as well as the code linting tools.

def create_web_user_from_consumer_user(username, password, ip=None, is_domain_admin=True):
now = datetime.utcnow()

new_user = WebUser.create(None, username, password, None, USER_CHANGE_VIA_WEB, is_admin=is_domain_admin)

Choose a reason for hiding this comment

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

how does this work without a domain? Or I guess having this set as None is this reason you get the "please create a domain" after you make this account?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Screenshot from 2021-03-10 19-26-10

Copy link
Owner Author

@challabeehyv challabeehyv Mar 10, 2021

Choose a reason for hiding this comment

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

Once the consumer user with out corresponding couch_user is logged in for the first time, we are creating the webuser and he sees the above screen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants