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

fix: set sms.createdBy consumer thread DHIS2-18187 [2.41] #18781

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Conversation

teleivo
Copy link
Contributor

@teleivo teleivo commented Oct 8, 2024

Issue

The UserDetails refactoring in 41 broke incoming SMS processing.

The sms processing code

Fix

  • Set the system user in the sms consumer thread.
  • Use stream instead of parallelStream as we would need to set the user on those threads as well. We used the same strategy on master in tracker. We are not sure how many people actually use the inbound sms feature in tracker.
  • Use the sms.createdBy user when setting fields like storedBy as it contains the user that sent the sms found/set by the SmsInboundController.getUserByPhoneNumber. The user that sent the sms is the one responsible for creating/updating/deleting events and other entities.

long smsId =
incomingSMSService.save(
message,
originator,
gateway,
receivedTime,
getUserByPhoneNumber(originator, message, currentUser));
return ok("Received SMS: " + smsId);
}
@PostMapping(consumes = APPLICATION_JSON_VALUE, produces = APPLICATION_JSON_VALUE)
@PreAuthorize("hasRole('ALL') or hasRole('F_MOBILE_SETTINGS')")
@ResponseBody
public WebMessage receiveSMSMessage(HttpServletRequest request, @CurrentUser User currentUser)
throws WebMessageException, IOException {
IncomingSms sms = renderService.fromJson(request.getInputStream(), IncomingSms.class);
sms.setCreatedBy(getUserByPhoneNumber(sms.getOriginator(), sms.getText(), currentUser));

Tests

I cannot backport the automated tests created on master during https://dhis2.atlassian.net/browse/DHIS2-17729. This would take weeks or more. I briefly tried backporting one but we cannot use controller tests as they cannot be non-transactional in 41. The sms are consumed in another thread so we have the issue of the test transaction isolating us from that thread.

Manual tests

this is the user that sent the sms and was found/set by the
SmsInboundController.getUserByPhoneNumber
@teleivo teleivo force-pushed the DHIS2-18187 branch 2 times, most recently from 943c467 to 3e5a301 Compare October 8, 2024 11:29
Copy link

sonarqubecloud bot commented Oct 8, 2024

@teleivo teleivo marked this pull request as ready for review October 8, 2024 12:43
@teleivo teleivo changed the title fix: set sms.createdBy consumer thread DHIS2-18187 fix: set sms.createdBy consumer thread DHIS2-18187 [2.41] Oct 9, 2024
@teleivo teleivo merged commit 42dc4d0 into 2.41 Oct 9, 2024
13 checks passed
@teleivo teleivo deleted the DHIS2-18187 branch October 9, 2024 08:43
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.

4 participants