refactor: resolve feedback for problem report

[nbAmit]

Resolve the feedback #560

Signed-off-by: Amit Padmani amit@northernblock.io

[TimoGlastra]

Nice! Really great improvements :)

Few nitpicks

[TimoGlastra]

The state won't change, so I think we should just call the connection repository with update directly as this will emit a state changed event.

[TimoGlastra]

Also, with the None state being removed, there is no way to determine that you received a problem report, so we should probably look into adding some events related to problem reports in follow up PRs

[TimoGlastra]

Same here

[nbAmit]

Hi @TimoGlastra, If we update the updateState with an update, it will be difficult to capture the event in the e2e test case?

https://github.com/hyperledger/aries-framework-javascript/blob/1041aa77bdc1c441d99e7852d4062766520d6597/packages/core/tests/credentials.test.ts#L483

[TimoGlastra]

Yes, see my comment above:

Also, with the None state being removed, there is no way to determine that you received a problem report, so we should probably look into adding some events related to problem reports in follow up PRs

We should probably add a new event (e.g. ConnectionProblemReportReceived, CredentialProblemReportReceived etc...) where the event looks something like:

{
   payload: {
     credentialRecord: { /* the record */ },
     problemReportMessage: { /* the problem report message */ }
}
}

[TimoGlastra]

You can either add it to the current PR, or you can bypass it by listening for the AgentMessageProcessedEvent for now and filtering on the problem report @type

[nbAmit]

@TimoGlastra , As of now I have removed the test case and I will add it into upcoming PR with the problem-report events

[TimoGlastra]

You also need to add a filter to the waitForProofRecordSubject and waitForCredentialRecordSubject methods for the errorMessage param to be taken into account

[nbAmit]

Sorry @TimoGlastra, I didn't get you.
errorMessage?: string | null
is it correct?

[TimoGlastra]

Sorry, let me rephrase.

Adding the errorMessage to the parameters of waitForProofRecord doesn't add errorMessage as a filter to the waitForProofRecord by itself. So passing errorMessage to the waitForProofRecord (as you do in the tests) doesn't do anything.

You would need to add an extra line to waitForProofRecordSubject:

.filter((e) => errorMessage === undefined || e.payload.proofRecord.errorMessage === errorMessage),

same for credentials

[codecov-commenter]

Codecov Report

Merging #584 (e336e15) into main (4d7d3c1) will decrease coverage by 0.08%.
The diff coverage is 69.69%.

@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
- Coverage   86.39%   86.30%   -0.09%     
==========================================
  Files         286      288       +2     
  Lines        6063     6069       +6     
  Branches      893      896       +3     
==========================================
  Hits         5238     5238              
- Misses        822      828       +6     
  Partials        3        3              

Impacted Files	Coverage Δ
...e/src/modules/common/messages/CommonMessageType.ts	100.00% <ø> (ø)
.../src/modules/connections/models/ConnectionState.ts	100.00% <ø> (ø)
...es/core/src/modules/credentials/CredentialState.ts	100.00% <ø> (ø)
packages/core/src/modules/proofs/ProofState.ts	100.00% <ø> (ø)
packages/core/src/modules/proofs/ProofsModule.ts	80.55% <ø> (ø)
.../modules/connections/services/ConnectionService.ts	88.31% <25.00%> (-2.46%)	⬇️
packages/core/src/agent/MessageReceiver.ts	81.44% <50.00%> (-0.56%)	⬇️
...rc/decorators/signature/SignatureDecoratorUtils.ts	100.00% <100.00%> (ø)
...modules/connections/repository/ConnectionRecord.ts	98.46% <100.00%> (ø)
...modules/credentials/repository/CredentialRecord.ts	91.22% <100.00%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d7d3c1...e336e15. Read the comment docs.

[TimoGlastra]

LGTM @nbAmit