Page MenuHomePhabricator

Dashboard integration: Add code for calling the P&E dashboard API
Closed, ResolvedPublic5 Estimated Story Points

Description

Once T317705, T331884, and T320434 are done, we want to write the code for actually calling the P&E dashboard API, although this won't be available to users just yet.

Note: a list of endpoints and their specs is available in the task description of T302584.

Acceptance criteria
  • Write code that implements T331884 and T320434 for the specific case of the P&E dashboard. The validation side should be implemented by passing the dry_run parameter to the API endpoints, and the actual actions should be performed by calling the same endpoints without the dry_run parameter. The following scenarios should be covered:
    • Registration enabled -> sync event
    • Registration updated -> unsync previous course, sync the new one
    • Registration disabled -> unsync event
    • Changes to the list of participants (add self, remove self, remove many, change visibility) -> sync participants
  • Add translations of the possible error messages listed in the task description of T302584.
  • Make sure that API requests use the standard proxy ($wgCopyUploadProxy) in production
  • If organizer enables registration without tracking tools, then someone registers for the event, and then the organizer adds a tracking tool, then the list of participants should also be synced immediately with the tool
Out of scope
  • User interface and API endpoints should not expose anything about tracking tools yet
Key points from meeting with Sage
  • We can use either a batched job or "live" updates (via DeferredUpdate), whatever works best for us.
  • Watch out for race conditions when sending updates
  • The full list of participants may be potentially very long; make sure this is not a problem.
  • Private registrants should NOT be synced with the dashboard; we will create another task about making sure that this is explained transparently in the interface.

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptSep 13 2022, 8:29 PM
Daimona changed the point value for this task from 3 to 5.
ifried renamed this task from Add code for calling the P&E dashboard API to [CAN COME AFTER NOV RELEASE] Add code for calling the P&E dashboard API.Nov 11 2022, 4:09 PM
ifried renamed this task from [CAN COME AFTER NOV RELEASE] Add code for calling the P&E dashboard API to Dashboard integration: Add code for calling the P&E dashboard API.Nov 11 2022, 4:24 PM

Change 921062 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Start implementing logic for the WikiEdu Dashboard

https://gerrit.wikimedia.org/r/921062

Change 921076 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Implement the remaining actions for the WikiEdu Dashboard

https://gerrit.wikimedia.org/r/921076

Change 921062 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Start implementing logic for the WikiEdu Dashboard

https://gerrit.wikimedia.org/r/921062

Change 921076 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Implement the remaining actions for the WikiEdu Dashboard

https://gerrit.wikimedia.org/r/921076

Daimona removed a project: Patch-For-Review.
Daimona added a subscriber: vaughnwalters.

@vaughnwalters This task is about the core functionality for the P&E dashboard integration. You can report any general issue with the integration itself here, but note that this is not testable yet because the functionality is not exposed anywhere. You will be able to test this once T317709 is done.

I've just added a new AC that I completely forgot about when writing the code:

If organizer enables registration without tracking tools, then someone registers for the event, and then the organizer adds a tracking tool, then the list of participants should also be synced immediately with the tool

Technically it's not "new", just something we've always known but I forgot to implement. I'll work on it now.

Change 925917 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Sync event participants when linking a WikiEduDashboard

https://gerrit.wikimedia.org/r/925917

@Ragesoss Hi, one thing I just realised: when passing the dry_run parameter, in particular to the confirm_event_sync endpoint, we may not have an event ID (e.g., if it's a new event). The endpoint works just fine if I omit the event_id parameter when dry_run is true, but I thought maybe you wanted to clarify that in the endpoint documentation. Thanks!

Change 930796 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Fix crash with event ID when syncing a new event with the Dashboard

https://gerrit.wikimedia.org/r/930796

Change 930796 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Fix crash with event ID when syncing a new event with the Dashboard

https://gerrit.wikimedia.org/r/930796

Change 925917 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Sync participants when linking a WikiEduDashboard to an existing event

https://gerrit.wikimedia.org/r/925917

Daimona removed a project: Patch-For-Review.

Moving back to QA, but the same note in T317707#8875052 (i.e., not testable yet) still applies.

Also noting what I wrote in T317707#8938682 about the event ID parameter being optional in dry run.

@Ragesoss Hi, one thing I just realised: when passing the dry_run parameter, in particular to the confirm_event_sync endpoint, we may not have an event ID (e.g., if it's a new event). The endpoint works just fine if I omit the event_id parameter when dry_run is true, but I thought maybe you wanted to clarify that in the endpoint documentation. Thanks!

Good catch! It would currently (seem to) work if you did an event sync without an event_id, but it should return an error code instead.

Is there a reason you'd want to do a dry_run in production without an an event_id? I could throw the error only when dry_run is false, but it makes more sense to me to make the dry_run throw as many of the same errors as possible compared to a real sync.

@Ragesoss Hi, one thing I just realised: when passing the dry_run parameter, in particular to the confirm_event_sync endpoint, we may not have an event ID (e.g., if it's a new event). The endpoint works just fine if I omit the event_id parameter when dry_run is true, but I thought maybe you wanted to clarify that in the endpoint documentation. Thanks!

Is there a reason you'd want to do a dry_run in production without an an event_id? I could throw the error only when dry_run is false, but it makes more sense to me to make the dry_run throw as many of the same errors as possible compared to a real sync.

Yup: when enabling or editing an event registration, we run the Dashboard validation before saving the event. In particular, if the event is being created, we don't have an event ID yet.

Also, @vaughnwalters: this can be tested now, as the endpoints were updated in T317709. As noted in T317707#8875052:

This task is about the core functionality for the P&E dashboard integration. You can report any general issue with the integration itself here

Yup: when enabling or editing an event registration, we run the Dashboard validation before saving the event. In particular, if the event is being created, we don't have an event ID yet.

Great. I've added a new error for if the ID is missing and it's not a dry_run, I've updated the doc at https://phabricator.wikimedia.org/T302584, and I've deployed it to staging. I should make sure to deploy it to Dashboard production before this lands in Event Center production, but it shouldn't change the expected behavior anyway, as all non-dry_run calls will include an ID (I assume).

Change 931684 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Add handling for new `missing_event_id` Dashboard error

https://gerrit.wikimedia.org/r/931684

Change 932349 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Send updates to the dashboard when a public participant becomes private

https://gerrit.wikimedia.org/r/932349

Change 932349 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Send updates to the dashboard when a public participant becomes private

https://gerrit.wikimedia.org/r/932349

Change 931684 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Add handling for new `missing_event_id` Dashboard error

https://gerrit.wikimedia.org/r/931684

Heads up @ifried everything in here is tested in other tickets so nothing else to check out for this ticket, but passing along to product sign off anyway.