Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev #83

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Dev #83

wants to merge 6 commits into from

Conversation

boostMNK
Copy link

Hi Heiner and Folke,

I really like your project and would love to contribute to it.
this is my first pull request to another project. Please let me know if I did something wrong or what you liked / disliked.

I've added the functionality to the admin-ui to update and add bookings by an admin.
Especially if bookings in past would be allowed for an admin it would in following use cases:

  • a user used a space but forgot to book it. And admin can create the booking for the user
  • due to illness a user can not keep the booking of 5 days, from which he attended the first 3 days. An Admin adjusts the date of the booking without having to delete and recreate a booking for the first 3 days.
  • a user booked the wrong space. An admin updates the booking with the space accordingly.

Cheers,
Sebastian

@virtualzone
Copy link
Collaborator

Hi @boostMNK,

thanks for your contribution! Unfortunately, some server tests are failing when testing your code.

The root cause seems to be this condition in line 395 in booking-router.go:

if bookForUser == nil || err != nil {

Could you check this and make sure the test cases pass? Thanks!

@boostMNK
Copy link
Author

boostMNK commented Feb 7, 2023

Hi @virtualzone ,

Excuse me, I could not find any time to fix it until now.
It seems to be solved by now. Can you confirm?

return "", errors.New("InternalServerError")
}
if !GetOrganizationRepository().isValidEmailForOrg(userEmail, org) {
fmt.Println("GetOrganizationRepository().isValidEmailForOrg(userEmail, org): bad request", userEmail, org)

Check failure

Code scanning / CodeQL

Log entries created from user input

This log entry depends on a [user-provided value](1).
@virtualzone
Copy link
Collaborator

@boostMNK Unfortunately, the problem doesn't seem to be solved yet. The server tests are still failing (you can run them locally by executing go test -v in the server folder). I'll take a look and try to help you during the next days.

@boostMNK
Copy link
Author

The problem occurred while checking for the http response if the booking is updated by non-admin user. As I believe the expected response should be forbidden I changed it accordingly. But this triggered several other errors in further tests.

@virtualzone
Copy link
Collaborator

virtualzone commented Feb 25, 2023

Hi @boostMNK ,
I just tried to fix the issues still causing the server tests to fail. However, there are some more issues with this PR which I can't fix quickly.

Critical issues:

  • packages react-date-picker and react-datetime-picker not installed in admin-ui's package.json
  • commons/ts/types/Booking.ts introduces a new method update() which is not used in any other types (all other types just use save() and Ajax.saveEntity() modifies the REST URL accordingly)
  • server tests are still failing
  • checking space availability is not working correctly (after adjusting the enter and leave times, free spaces cannot be selected)
  • translations are missing in the new edit/add booking dialog
  • when trying to book a space with parameters which are not allowed (i.e. time frame too long), there is no convenient error handling telling the user what to do to correct his error)
  • When deleting a booking and not confirming the process, user is redirected to the edit booking dialog instead of staying at the booking list
  • No new tests were added to booking-router_test.go, ensuring that only (space) admins can add/modify/delete a user's bookings
  • The permission checks in booking-router.go functions getOne() and update() are insufficient (i.e. update() not only allows space admins to update bookings, not user themselves as it was implemented before)
  • There a CodeQL issues in booking-router.go due to log entries created from user inputs

Other issues I noticed:

  • New dialog to add a booking should rather use a auto-completing search field for selecting a user than a drop down (could become large)
  • enter and leaving times should be pre-set with useful values

I'd be happy to merge your pull request. However, especially the critical issues should be addressed first.

Many thanks for your efforts!

@boostMNK
Copy link
Author

Hi @virtualzone, unfortunately I was unable to continue working on these issues.
But now I had some time to solve the critical issues you mentioned. Please tell me if something is not working as expected or missing.

I try to spend some time on

  • New dialog to add a booking should rather use a auto-completing search field for selecting a user than a drop down (could become large)

the next days.

Cheers, boost

fashberg pushed a commit to fashberg/seatsurfing-backend that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants