credo-ts icon indicating copy to clipboard operation
credo-ts copied to clipboard

feat: Action Menu protocol (Aries RFC 0509) implementation

Open genaris opened this issue 3 years ago • 3 comments

Implementation of module fully supporting Aries RFC 0509 for both requester and responder roles.

Protocol states and information are stored in records of type ActionMenuRecord. As this protocol currently defines a single active menu per connection, there will be at most 2 records per connection (as theoretically is possible to serve as requester and responder at the same time). While this information could be stored also in Connection Metadata, specific records were created thinking in a future version/extension of this protocol where multiple menus could be deployed (e.g. a menu by thread).

Still plenty of tests to do and fixes/support for error messages, as well as check compatibility with ACA-Py implementation.

Solves #471 Signed-off-by: Ariel Gentile [email protected]

genaris avatar Aug 06 '22 23:08 genaris

Codecov Report

Merging #974 (c8fafbd) into main (f487182) will increase coverage by 0.28%. The diff coverage is 94.86%.

@@            Coverage Diff             @@
##             main     #974      +/-   ##
==========================================
+ Coverage   88.06%   88.35%   +0.28%     
==========================================
  Files         492      520      +28     
  Lines       11623    12071     +448     
  Branches     1936     1992      +56     
==========================================
+ Hits        10236    10665     +429     
- Misses       1326     1343      +17     
- Partials       61       63       +2     
Impacted Files Coverage Δ
...modules/action-menu/models/ActionMenuOptionForm.ts 53.84% <53.84%> (ø)
...ction-menu/models/ActionMenuOptionFormParameter.ts 55.55% <55.55%> (ø)
...src/modules/action-menu/models/ActionMenuOption.ts 88.88% <88.88%> (ø)
...modules/action-menu/repository/ActionMenuRecord.ts 94.11% <94.11%> (ø)
...re/src/modules/action-menu/messages/MenuMessage.ts 95.65% <95.65%> (ø)
.../modules/action-menu/services/ActionMenuService.ts 97.98% <97.98%> (ø)
...s/core/src/modules/action-menu/ActionMenuModule.ts 98.07% <98.07%> (ø)
packages/core/src/agent/Agent.ts 95.40% <100.00%> (+0.05%) :arrow_up:
...s/core/src/modules/action-menu/ActionMenuEvents.ts 100.00% <100.00%> (ø)
...ges/core/src/modules/action-menu/ActionMenuRole.ts 100.00% <100.00%> (ø)
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Aug 07 '22 00:08 codecov-commenter

Nice and tidy 👍 . My preference would go to making this an optional add-on module. However I'm fine with merging it in core in 0.2.0 and then extracting it from core into a separate package (like module-tenants) in 0.3.0. What do you think?

Edit: I see unit tests are missing, but that's probably still todo as you mention in the pr description?

Thanks! I think it would be a good idea as you say, we can merge it in 0.2.x and then move it to a separate package, as it is not a core protocol.

I'm writing the unit tests and actually found a few issues, so I'll fix them and submit the PR for review shortly (hopefully 😄).

genaris avatar Aug 13 '22 20:08 genaris

Well now I've added some error handling and tests (with our new friend SonarCloud complaining) I think it's not as nice and tidy as before, but let's see how it goes 😄 . I updated PR description with some more details about the work done and what's left.

genaris avatar Aug 18 '22 05:08 genaris

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 71 Code Smells

No Coverage information No Coverage information
6.8% 6.8% Duplication

sonarqubecloud[bot] avatar Aug 19 '22 17:08 sonarqubecloud[bot]

I'm a bit hesitant to merge this now as it will be quite a hassle to merge into 0.3.0-pre afterwards. When I get the time to immediately update the implementation to work with 0.3.0-pre I'll merge

TimoGlastra avatar Aug 26 '22 19:08 TimoGlastra

I'm a bit hesitant to merge this now as it will be quite a hassle to merge into 0.3.0-pre afterwards. When I get the time to immediately update the implementation to work with 0.3.0-pre I'll merge

Do you mean to merge this module directly on 0.3.0-pre? Or do you want to first update 0.3.0-pre with current main branch status and then merge this one into main?

genaris avatar Aug 26 '22 21:08 genaris

Do you mean to merge this module directly on 0.3.0-pre? Or do you want to first update 0.3.0-pre with current main branch status and then merge this one into main?

No it's probably good to have it in 0.2.x if possible, but after it has been merged in main we need to rebase the 0.3.0-pre branch and integrate the changes from main. I'll do that soon, and then we can make an additional PR to extract the action menu into a separate package.

TimoGlastra avatar Aug 30 '22 08:08 TimoGlastra

Can you update once more? Then I'll merge this PR

TimoGlastra avatar Sep 01 '22 09:09 TimoGlastra