search

tl-its-umich-edu / canvas-course-manager-next

Canvas Course Manager Next: A redesign of the existing CCM application. It extends Canvas features, makes cumbersome features easier to use, and adds new features.
8 stars 9 forks source link

Introduce `createLimitedPromises` wrapper for `p-limit` (#326) #327

Closed ssciolla closed 2 years ago

ssciolla commented 2 years ago

The PR aims to resolve #326.

(I'm pretty sure this does the trick, and it kinda feels like magic 🪄 .)

Resource(s):

pushyamig commented 2 years ago

I will review this some time in late afternoon.

pushyamig commented 2 years ago

I did some testing with 400 user enrolling 4 times and in all the time i saw the success. here is the report from that. I might do testing on Monday. Screen Shot 2022-02-04 at 4 17 42 PM

pushyamig commented 2 years ago

I will review some more by end of day

ssciolla commented 2 years ago

@pushyamig and @lsloan, I think this is ready for review. Please let me know if any of the cases where I introduced limiting you think it should be removed (fine doing that, just want your input; I weighed in on the cases where it seemed less useful).

pushyamig commented 2 years ago

The change seems to be straight forward but affects most features. I did some preliminary testing and review, things looks good! I will go one more round tomorrow and will approve.

pushyamig commented 2 years ago

Over all this is a positive change and huge performance improvement and increase customer satisfaction rate. This also means that for Add Non-UM user we could support more that 200 (based on testing).

I have my testing report across features

This is a go from me, I think just need to resolve some merge conflicts and probably remove the concurrency call with un merge sections

ssciolla commented 2 years ago

@pushyamig, thanks for the thorough review!