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

Flexible mqtt topics #1850

Merged
merged 8 commits into from
Jul 28, 2020
Merged

Flexible mqtt topics #1850

merged 8 commits into from
Jul 28, 2020

Conversation

lincmba
Copy link
Contributor

@lincmba lincmba commented Jul 20, 2020

Changes / Features implemented

Supercedes #1793

  1. Remove send message from post save signal.
    • For forms, this is called each time there is an update to the form yet the notification is only interested in changes in the form xls file itself.
    • For submissions, this isn't called when there is a bulk update of the submissions.
  2. Forms: Only send a notification when the form xls has been updated
    Submissions: Send notification both upon creation and edit
  3. Send notification upon bulk deletion during a csv import.
  4. Send notification on bulk submission review
  5. Change topics to allow more flexibility. The topics will be of the form:
    /onadata/organization/{org_id}/project/{proj_id}/xform/{xform_id}/#

To test

  1. MQTT server
  • ssh to mqtt server
  • listen to any of the channels below
  • perform the relevant action (delete, create, edit ...)
  • a message payload should be posted on the channel
  1. API
  • perform the relevant action (delete, create, edit ...)
  • a message payload should be included in the relevant API json response
channel url action
/onadata/organization/[org_username]/project/[proj_id]/xform/[form_id]/submission/deleted /api/v1/messaging.json?target_type=xform&target_id={form_id}&verb=submission_deleted submission deletion
/onadata/organization/[org_username]/project/[proj_id]/xform/[form_id]/submission/created /api/v1/messaging.json?target_type=xform&target_id={form_id}&verb=submission_created submission creation
/onadata/organization/[org_username]/project/[proj_id]/xform/[form_id]/submission/edited /api/v1/messaging.json?target_type=xform&target_id={form_id}&verb=submission_edited submission edit
/onadata/organization/[org_username]/project/[proj_id]/xform/[form_id]/submission/reviewed /api/v1/messaging.json?target_type=xform&target_id={form_id}&verb=submission_review submission_review
/onadata/organization/[org_username]/project/[proj_id]/xform/[form_id]/form/updated /api/v1/messaging.json?target_type=xform&target_id={form_id}&verb=form_updated form updated

Example payload:

{
    "id": 113, 
    "verb": 
    "submission_deleted", 
    "message": {"id": ["35838"]}, 
    "user": "linc", 
    "timestamp": "2020-06-10T11:10:45.346353+00:00"
}

Steps taken to verify this change does what is intended

Written tests

Side effects of implementing this change

Closes #1846

@lincmba lincmba force-pushed the flexible-mqtt-topics branch 2 times, most recently from add35e2 to 4799710 Compare July 20, 2020 21:20
@lincmba lincmba requested review from ukanga and DavisRayM July 20, 2020 21:51
@lincmba lincmba force-pushed the flexible-mqtt-topics branch 6 times, most recently from c7caace to 2a92a35 Compare July 24, 2020 16:35
Comment on lines 713 to 717
# send form update notification
send_message(
instance_id=self.object.id, target_id=self.object.id,
target_type=XFORM, user=self.request.user,
message_verb=FORM_UPDATED)
Copy link
Contributor

@DavisRayM DavisRayM Jul 27, 2020

Choose a reason for hiding this comment

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

Is there a chance that the _try_update_xlsform function might fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mainly concerned with sending the notification before confirming that the XForm has been updated...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Made changes

Copy link
Contributor

@DavisRayM DavisRayM left a comment

Choose a reason for hiding this comment

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

LGTM, have a few comments. Also this might be a bit nitpicky but there are two commits named Use Organisational name could we squash those into one ?

Comment on lines +30 to +33
# Sometimes the Action isn't created yet, hence
# the need to delay 2 seconds
call_backend_async.apply_async(
(backend, instance.id, backend_options), countdown=2)
Copy link
Contributor

@DavisRayM DavisRayM Jul 27, 2020

Choose a reason for hiding this comment

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

Why are the actions not created at this point sometimes? Expecting this function to only be called after the Action is saved... and instance should be an Action object right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I expected the @use_master here decorator had this covered. This returned None when called on submission creation and submission edit but had an instance on when called in other places. Hence a message couldn't be sent here on submission creation or edit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mhh weird... I'm guessing the created boolean was also True at this point?

@lincmba lincmba force-pushed the flexible-mqtt-topics branch 2 times, most recently from 86e513b to 01b6167 Compare July 27, 2020 12:51
Copy link
Contributor

@DavisRayM DavisRayM left a comment

Choose a reason for hiding this comment

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

LGTM, let's wait for @ukanga or/and @ivermac to have a look

@lincmba lincmba requested a review from ivermac July 27, 2020 13:37
ukanga
ukanga previously approved these changes Jul 27, 2020
@faith-mutua
Copy link

@lincmba this has passed QA. A message payload is included for each API json response.

@faith-mutua faith-mutua added the QA+ PR passed QA testing label Jul 28, 2020
…each time there is an update to the form yet the notification is only interested in changes in the form xls file itself.

For submissions, this isn't called when there is a bulk update of the submissions.

Signed-off-by: lincmba <[email protected]>
For submissions: Send notification both upon creation and edit

Signed-off-by: lincmba <[email protected]>
Send notification on bulk submission review

Signed-off-by: lincmba <[email protected]>
`/onadata/organization/{org_id}/project/{proj_id}/xform/{xform_id}/#`

Signed-off-by: lincmba <[email protected]>
Signed-off-by: lincmba <[email protected]>
1. Submission Review
2. Bulk Submission Review
3. CSV upload submission creation
4. CSV Upload submission edit
5. Form update
6. Submission Delete
7. Bulk Submission Delete

Signed-off-by: lincmba <[email protected]>
Send message only after successful xls form update
@DavisRayM DavisRayM deleted the flexible-mqtt-topics branch July 28, 2020 11:03
@DavisRayM DavisRayM mentioned this pull request Jul 28, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA+ PR passed QA testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notifications API form_update topic is noisy
4 participants