JASPER-887: Court Location Panel: Set up job to extract the data from the spreadsheet and update the table entries - #1350
ronaldo-macapobre wants to merge 14 commits into
Conversation
…and update related tests
| } | ||
|
|
||
| [Fact] | ||
| public void GetSheet_Ignores_Unknown_Columns() |
There was a problem hiding this comment.
Note, this doesn't match the requirement:
the format of the spreadsheet is as expected (correct tabs and columns)
But to be fair, I don't understand the context behind that requirement. Is this really a case where we need to be strict and ignore excel documents that don't exactly conform? Or is this more of a "best effort" workflow?
There was a problem hiding this comment.
I implemented the ExcelParser and ExcelWorkBook to handle generic operations to excel files in case a new data extraction feature is required in the future. The behavior right now parses the spreadsheet on "best effort" and ignore the data it doesn't recognize.
In this unit test, "Region" is not a valid column that CourtLocation model recognize so rather than throw an error, it is just ignored.
| public static void Register(TypeAdapterConfig config) | ||
| { | ||
| config.NewConfig<ParserModel.CourtLocation, CourtLocationDto>() | ||
| .Map(dest => dest.IsStaffed, src => src.Staffed == "Staffed") |
There was a problem hiding this comment.
should this be "Staffed" or "Yes"?
There was a problem hiding this comment.
The spreadsheet data has either "Staffed" or "Unstaffed". To easily handle this in the frontend, I added a boolean IsStaffed property to interpret it easier.
| } | ||
|
|
||
| this.Logger.LogInformation("Court Location Attachment found."); | ||
| return attachments.First().Value; |
There was a problem hiding this comment.
we should probably get the attachment with the matching key and not just the first attachment, right? I feel like if we have multiple attachments where one has the right key this may not work.
There was a problem hiding this comment.
Ah make sense. I will update the code.
| } | ||
| } | ||
|
|
||
| public async Task<OperationResult> ReplaceCourtLocationsAsync(CourtLocationDto[] dtos) |
There was a problem hiding this comment.
this is potentially risky with the logic that doesn't validate missing columns/format, if we get an invalid/empty file will that clear out the court locations?
There was a problem hiding this comment.
Yes, the existing court locations will definitely be overwritten. However, since this feature is only used internally by the client, the chances of uploading an incorrect spreadsheet is very low and any mistakes are easily corrected by sending the another file and rerunning the job.
|
Question, should we validate the sender as well? it seems like our only "security" checks are filename + subject. Since this is an open source project, could someone take down the application by crafting a malicious email attachment + expected subject? |
| return attachments.First().Value; | ||
| } | ||
|
|
||
| private async Task<CourtLocationDto[]> GetCourtLocations(MemoryStream stream) |
There was a problem hiding this comment.
don't see any await in here, does this need to be async?
| .Setup(s => s.GetFilteredEmailsAsync(Mailbox, Subject, null, true)) | ||
| .ReturnsAsync([]); | ||
|
|
||
| var result = _job.Execute(); |
There was a problem hiding this comment.
you awaited this elsewhere, in this test file, should here as well.
|
Will approve pending responses to questions about security and column validation. |
Interesting thought. I will clarify if we could determine the sender of this email. I'll also add a file scanning step for added safety. |
Yes, only started thinking about that with this PR, but it's not like the JASPER inbox is VPN protected or anything like that, so as far as I know anyone could send a file to it. |
I'll probably use a pattern for the sender (e.g. |
…ce to support email sender patterns
|



Pull Request for JIRA Ticket: JASPER-887
Issue ticket number and link
https://jira.justice.gov.bc.ca/browse/JASPER-887
Description
Implements the job that checks and parses an email containing the Court Locations. If the parsing is successful, it would replace the existing court locations in the database and persist the new ones.
Type of change
How Has This Been Tested?
Checklist:
Screen Grab