-
Notifications
You must be signed in to change notification settings - Fork 54
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
Elisa/7492 public device endpoint #8341
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package gov.cdc.usds.simplereport.api.devicetype; | ||
|
||
public interface PublicDeviceType {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,8 @@ public class WebConfiguration implements WebMvcConfigurer { | |
public static final String PATIENT_UPLOAD = "/upload/patients"; | ||
public static final String RESULT_UPLOAD = "/upload/results"; | ||
public static final String CONDITION_AGNOSTIC_RESULT_UPLOAD = "/upload/condition-agnostic"; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. formatting nit - extra newline? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh thank you! I'll update that in the part 3 PR 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooo oops -- I'll remove this in my next PR 😅 Thank you for catching this 🎣 |
||
public static final String DEVICES = "/devices"; | ||
public static final String GRAPH_QL = "/graphql"; | ||
|
||
@Autowired private RestLoggingInterceptor _loggingInterceptor; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package gov.cdc.usds.simplereport.db.model; | ||
|
||
import com.fasterxml.jackson.annotation.JsonIgnore; | ||
import com.fasterxml.jackson.annotation.JsonView; | ||
import gov.cdc.usds.simplereport.api.devicetype.PublicDeviceType; | ||
import jakarta.persistence.CascadeType; | ||
import jakarta.persistence.Column; | ||
import jakarta.persistence.Entity; | ||
|
@@ -23,26 +24,31 @@ | |
public class DeviceType extends EternalAuditedEntity { | ||
|
||
@Column(nullable = false) | ||
@JsonView(PublicDeviceType.class) | ||
private String name; | ||
|
||
@Column(nullable = false) | ||
@JsonView(PublicDeviceType.class) | ||
private String manufacturer; | ||
|
||
@Column(nullable = false) | ||
@JsonView(PublicDeviceType.class) | ||
private String model; | ||
|
||
@JoinTable( | ||
name = "device_specimen_type", | ||
joinColumns = @JoinColumn(name = "device_type_id"), | ||
inverseJoinColumns = @JoinColumn(name = "specimen_type_id")) | ||
@OneToMany(fetch = FetchType.LAZY) | ||
@JsonView(PublicDeviceType.class) | ||
private List<SpecimenType> swabTypes; | ||
|
||
@Column(nullable = false) | ||
@JsonView(PublicDeviceType.class) | ||
private int testLength; | ||
|
||
@JsonIgnore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @emyl3 Have you looked into our concerns that removing these ignore tags will cause an issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And I apologize because I just now realized I never added that detail to the ticket: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had previously deployed this branch both with and without the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if this is not what you had in mind and I can adjust the testing plan and retest 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part of me thinks that the issue is no longer relevant because the audit log was moved out of our db table. The other part of me thinks if it is somehow still an issue, we aren't going to see it until we send this to prod. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I talked to @mehansen about this a bit and I loved her idea of just trying to replicate the original issue as a means of confirming that its no longer a concern. But again, the lack of an audit log in the db might make that a futile effort. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After discussing with @DanielSass @mehansen and @mpbrown after standup, we will deploy this after the cert renewal. Once merged in, I will keep an eye on the following:
If there are any slowdowns noticeable from Azure following the deploy, we will revert this PR and see if there are ways we can investigate this further. In the meantime, I will look into if there's a way to view DB request times in Azure or check-in with Shanice after cert renewal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm seeing the our "Azure Database for PostgreSQL - Flexible Server" view for our prod DB and there is a way to view both the Read and Write Throughput Bytes/Sec (Avg) -- Would this be sufficient? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shanice-skylight Are you familiar with any Azure tools to monitor avg DB request times? I saw there's this but we don't seem to have that enabled. 🤔 |
||
@OneToMany(mappedBy = "deviceTypeId", cascade = CascadeType.ALL, orphanRemoval = true) | ||
@JsonView(PublicDeviceType.class) | ||
List<DeviceTypeDisease> supportedDiseaseTestPerformed = new ArrayList<>(); | ||
|
||
protected DeviceType() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package gov.cdc.usds.simplereport.service; | ||
|
||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.beans.factory.annotation.Value; | ||
import org.springframework.security.access.AccessDeniedException; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
/** Service to fetch and save DeviceTypes from our prod env */ | ||
@Service | ||
@Slf4j | ||
@Transactional(readOnly = true) | ||
public class DeviceTypeProdSyncService { | ||
@Value("${simple-report.production.devices-token}") | ||
private String token; | ||
|
||
public boolean validateToken(String headerToken) throws AccessDeniedException { | ||
if (token.equals(headerToken)) { | ||
return true; | ||
} | ||
throw new AccessDeniedException("Access denied"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
package gov.cdc.usds.simplereport.api; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.Mockito.when; | ||
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; | ||
|
||
import gov.cdc.usds.simplereport.service.DeviceTypeProdSyncService; | ||
import gov.cdc.usds.simplereport.test_util.TestUserIdentities; | ||
import org.json.JSONArray; | ||
import org.json.JSONObject; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.boot.test.mock.mockito.MockBean; | ||
import org.springframework.http.MediaType; | ||
import org.springframework.mock.web.MockHttpServletResponse; | ||
import org.springframework.security.access.AccessDeniedException; | ||
import org.springframework.test.web.servlet.MockMvc; | ||
import org.springframework.test.web.servlet.MvcResult; | ||
import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; | ||
|
||
class DeviceTypeControllerTest extends BaseFullStackTest { | ||
|
||
@Autowired private MockMvc _mockMvc; | ||
@MockBean private DeviceTypeProdSyncService _mockDeviceTypeProdSyncService; | ||
|
||
@BeforeEach | ||
void init() { | ||
TestUserIdentities.withStandardUser( | ||
() -> { | ||
_dataFactory.initGenericDeviceTypeAndSpecimenType(); | ||
}); | ||
} | ||
|
||
@Test | ||
void getDevices_withValidateToken_success() throws Exception { | ||
when(_mockDeviceTypeProdSyncService.validateToken(any())).thenReturn(true); | ||
MockHttpServletRequestBuilder builder = | ||
get(ResourceLinks.DEVICES) | ||
.contentType(MediaType.valueOf(MediaType.APPLICATION_JSON_VALUE)) | ||
.accept(MediaType.APPLICATION_JSON) | ||
.characterEncoding("UTF-8"); | ||
|
||
MvcResult result = this._mockMvc.perform(builder).andReturn(); | ||
MockHttpServletResponse res = result.getResponse(); | ||
|
||
assertThat(res.getStatus()).isEqualTo(200); | ||
|
||
JSONArray jsonRes = new JSONArray(res.getContentAsString()); | ||
assertThat(jsonRes.length()).isEqualTo(1); | ||
|
||
JSONObject deviceType = jsonRes.getJSONObject(0); | ||
assertThat(deviceType.getString("manufacturer")).isEqualTo("Acme"); | ||
assertThat(deviceType.getString("model")).isEqualTo("SFN"); | ||
assertThat(deviceType.getString("name")).isEqualTo("Acme SuperFine"); | ||
assertThat(deviceType.getInt("testLength")).isEqualTo(15); | ||
assertThat(deviceType.getJSONArray("supportedDiseaseTestPerformed")).isEmpty(); | ||
// ensure deviceType internalId is not returned | ||
assertTrue(deviceType.isNull("internalId")); | ||
|
||
JSONArray swabTypes = deviceType.getJSONArray("swabTypes"); | ||
assertThat(swabTypes.length()).isEqualTo(1); | ||
JSONObject swabType = swabTypes.getJSONObject(0); | ||
assertThat(swabType.getString("collectionLocationCode")).isEqualTo("986543321"); | ||
assertThat(swabType.getString("collectionLocationName")).isEqualTo("Da Nose"); | ||
assertThat(swabType.getString("name")).isEqualTo("Nasal swab"); | ||
assertThat(swabType.getString("typeCode")).isEqualTo("000111222"); | ||
// ensure swabType internalId is not returned | ||
assertTrue(swabType.isNull("internalId")); | ||
} | ||
|
||
@Test | ||
void getDevices_withValidateToken_failure() throws Exception { | ||
when(_mockDeviceTypeProdSyncService.validateToken(any())) | ||
.thenThrow(new AccessDeniedException("Bad token")); | ||
MockHttpServletRequestBuilder builder = | ||
get(ResourceLinks.DEVICES) | ||
.contentType(MediaType.valueOf(MediaType.APPLICATION_JSON_VALUE)) | ||
.accept(MediaType.APPLICATION_JSON) | ||
.characterEncoding("UTF-8"); | ||
|
||
MvcResult result = this._mockMvc.perform(builder).andReturn(); | ||
MockHttpServletResponse res = result.getResponse(); | ||
|
||
assertThat(res.getStatus()).isEqualTo(401); | ||
assertThat(res.getContentAsString()).isEmpty(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package gov.cdc.usds.simplereport.service; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
||
import org.junit.jupiter.api.Test; | ||
import org.springframework.beans.factory.annotation.Value; | ||
import org.springframework.security.access.AccessDeniedException; | ||
|
||
class DeviceTypeProdSyncServiceTest extends BaseServiceTest<DeviceTypeProdSyncService> { | ||
@Value("${simple-report.production.devices-token}") | ||
private String token; | ||
|
||
@Test | ||
void validateToken_withValidToken_success() { | ||
assertThat(_service.validateToken(token)).isTrue(); | ||
} | ||
|
||
@Test | ||
void validateToken_withInvalidToken_throwsException() { | ||
assertThrows(AccessDeniedException.class, () -> _service.validateToken("foo")); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea! I got tripped up w/ these names