uni icon indicating copy to clipboard operation
uni copied to clipboard

Feature/cancel reservations

Open jlcrodrigues opened this issue 2 years ago • 8 comments

Closes #656 Depends on #658 , #1073

Added functionality to cancel library reservations.

reservation

Review checklist

  • [x] Terms and conditions reflect the current change
  • [x] Contains enough appropriate tests
  • [x] If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • [x] Properly adds an entry in changelog.md with the change
  • [x] If PR includes UI updates/additions, its description has screenshots
  • [x] Behavior is as expected
  • [x] Clean, well-structured code

jlcrodrigues avatar Apr 26 '23 14:04 jlcrodrigues

@LuisDuarte1

Good progress! Looks clean to me, but i have some UX issues at least on my testing.

First when I cancel, and it is successful, the UI blocks completely, at least for me, when the toast message shows up on the screen. There are no errors in the logs so it's a bit strange, but try to replicate it and let me know.

Second, the call to sigarra is really slow, even on the web at least for me, and maybe a circular progress indicator should help the user to not get frustrated.

  1. I actually reiterated this on the feature/create-reservations and forgot to add it here. The block is due to the common error with toast messages (#783 ).
  2. The lag has been consistent on the website for me as well. My solution was to create an initial info toast message (e.g. Cancelling reservation...) that would then be followed by the actual confirmation one that you saw.

Let me know what you think and I will try to fix the errors asap.

jlcrodrigues avatar Jun 28 '23 15:06 jlcrodrigues

  1. The lag has been consistent on the website for me as well. My solution was to create an initial info toast message (e.g. Cancelling reservation...) that would then be followed by the actual confirmation one that you saw.

As long if you block the user from making more requests to the same reservation I think this is a good approach

LuisDuarte1 avatar Jun 28 '23 16:06 LuisDuarte1

Any reason for not merging this upstream directly?

bdmendes avatar Jul 22 '23 18:07 bdmendes

Any reason for not merging this upstream directly?

Sometime ago we decided in a meet this feature makes sense releasing when there is the possiblity to make reservations, which is the next step after this PR.

thePeras avatar Jul 22 '23 18:07 thePeras

I see, but this brings some issues: one can merge this by mistake since the branch is not protected; also being based on another branch means that conflicts will have to be handled all at once once everything gets merged later on. Although we are not fully trunk-based, since we have a develop, I don't think we should keep many develops around. It's something to consider next meeting, for sure.

bdmendes avatar Jul 22 '23 19:07 bdmendes

Codecov Report

Attention: Patch coverage is 5.08021% with 355 lines in your changes missing coverage. Please review.

Project coverage is 16%. Comparing base (9bbf5d4) to head (8ccb573). Report is 2178 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #795    +/-   ##
=======================================
- Coverage       17%    16%    -0%     
=======================================
  Files          207    216     +9     
  Lines         6418   6727   +309     
=======================================
+ Hits          1037   1058    +21     
- Misses        5381   5669   +288     
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Sep 26 '23 16:09 codecov[bot]

I've changed the target and this will now be merged to develop after #1073

jlcrodrigues avatar Jan 08 '24 07:01 jlcrodrigues

@bdmendes Fixed the tests and made changes according to your suggestions.

Also, don't forget #1073 is to be merged first.

jlcrodrigues avatar Jan 24 '24 22:01 jlcrodrigues

Thanks for your work here. This got a little deprecated with all the updates we made to the app. I'm closing this PR but this logic will be useful in the future, if we want to add this feature.

DGoiana avatar May 20 '25 15:05 DGoiana