add didUpdateRolePermissions event

[petersg83]

No description provided.

[Convly]

LGTM, waiting for the tests

[alexandrebodin]

I will review it before it is merged

[codecov]

Codecov Report

Merging #8333 into master will increase coverage by 0.07%.
The diff coverage is 98.11%.

@@            Coverage Diff             @@
##           master    #8333      +/-   ##
==========================================
+ Coverage   33.19%   33.27%   +0.07%     
==========================================
  Files        1220     1220              
  Lines       13618    13625       +7     
  Branches     1357     1357              
==========================================
+ Hits         4521     4534      +13     
+ Misses       8212     8207       -5     
+ Partials      885      884       -1     

Flag	Coverage Δ
#front	24.72% <ø> (ø)
#unit	54.69% <98.11%> (+0.23%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/strapi-admin/services/permission.js	71.64% <ø> (-5.93%)	⬇️
packages/strapi-admin/services/role.js	96.99% <98.00%> (+2.67%)	⬆️
packages/strapi-admin/controllers/role.js	47.16% <100.00%> (ø)
packages/strapi-admin/services/metrics.js	100.00% <100.00%> (ø)

---

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 fad9fda...54f83ea. Read the comment docs.

[alexandrebodin]

It would be simpler to send the event from the service as it is where the decision can be made without leaking internal state. to the caller (controller in this case).

Another approach (I like it but I think it's to soon) is to use the eventHub to send a didAssignPermission like event and have a listener that will check the event and send telemetry if necessary

[petersg83]

It was in the service at the begining but @Convly pointed some problems with that : #8333 (comment)
What do you think about that?

[alexandrebodin]

It was in the service at the begining but @Convly pointed some problems with that : #8333 (comment)
What do you think about that?

We can check that the existingPermissions count is > 0 to send the event I believe

[petersg83]

We can check that the existingPermissions count is > 0 to send the event I believe

We would miss the edge case where a role has, at some point, 0 permissions and is then updated with some.
If you're ok with that because you find the code cleaner, I'm ok with that too.

[petersg83]

I think the best place would still have been in the controller. As it is a service, anyone can use it in a plugin or else. So the logic way for this telemetry would be, for me, only with the route which the admin panel calls.

[alexandrebodin]

I'm going to be picky here but I think this need a refactoring.

We should have the assignPermissions and addPermissions methods in the role service and those could use the permissions service for createMany (which would not have a role there) Let's decouple those two services

[alexandrebodin]

last comment :)

[alexandrebodin]

LGTM