Feature/cancel reservations
Closes #656 Depends on #658 , #1073
Added functionality to cancel library reservations.

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.mdwith the change - [x] If PR includes UI updates/additions, its description has screenshots
- [x] Behavior is as expected
- [x] Clean, well-structured code
@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.
- I actually reiterated this on the
feature/create-reservationsand forgot to add it here. The block is due to the common error with toast messages (#783 ). - 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.
- 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
Any reason for not merging this upstream directly?
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.
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.
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.
I've changed the target and this will now be merged to develop after #1073
@bdmendes Fixed the tests and made changes according to your suggestions.
Also, don't forget #1073 is to be merged first.
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.