Description
npg_set_state is a function invoked by the npg_actions assets controller with the purpose of releasing a batch if all the requests have been qc'd. During the rails 6.1 upgrade an issue was identified with the tests for this behaviour (story here) and highlighted that this method may not be working completely as intended. The tests were false passing as both the valid request and cancelled request had the same BatchRequest position due to factory bot concurrency errors which caused the cancelled request to not save and thus not associated with the batch. So when the all_requests_qced? was run the cancelled request was not being included meaning the test was not testing as intended.
The tests were updated to correctly reflect the existing behaviour (seen here) but we would like to investigate whether this behaviour is correct.
The original tests suggest that a batch should be released by this function if it contains a valid (failed or passed) request and a cancelled request. However this is currently not happening as cancelled batch requests are not being filtered out here so the batch does not release as the cancelled request doesn't have the correct events associated with it.
Code snippet for context
def all_requests_qced?
requests.all? { |request| request.asset.resource? || request.events.family_pass_and_fail.exists? }
end
The fix for this and the potential correct behaviour would be to remove cancelled requests from this check like this:
def all_requests_qced?
requests.not_cancelled.all? { |request| request.asset.resource? || request.events.family_pass_and_fail.exists? }
end
There is limited understanding around this npg_action but this behaviour should be confirmed with NPG if possible and the tests should be updated to correctly reflect the changes.
Who the primary contacts are for this work
Dasun P, Tom W, Ben T, Andrew S
Knowledge or Stake holders
Dasun P, Tom W, Ben T, Andrew S
Description npg_set_state is a function invoked by the npg_actions assets controller with the purpose of releasing a batch if all the requests have been qc'd. During the rails 6.1 upgrade an issue was identified with the tests for this behaviour (story here) and highlighted that this method may not be working completely as intended. The tests were false passing as both the valid request and cancelled request had the same BatchRequest position due to factory bot concurrency errors which caused the cancelled request to not save and thus not associated with the batch. So when the all_requests_qced? was run the cancelled request was not being included meaning the test was not testing as intended. The tests were updated to correctly reflect the existing behaviour (seen here) but we would like to investigate whether this behaviour is correct.
The original tests suggest that a batch should be released by this function if it contains a valid (failed or passed) request and a cancelled request. However this is currently not happening as cancelled batch requests are not being filtered out here so the batch does not release as the cancelled request doesn't have the correct events associated with it.
Code snippet for context
The fix for this and the potential correct behaviour would be to remove cancelled requests from this check like this:
There is limited understanding around this npg_action but this behaviour should be confirmed with NPG if possible and the tests should be updated to correctly reflect the changes.
Who the primary contacts are for this work Dasun P, Tom W, Ben T, Andrew S
Knowledge or Stake holders Dasun P, Tom W, Ben T, Andrew S