Skip to content

implement local event handling #79

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

Closed

Conversation

imcaizheng
Copy link
Contributor

Notes

  • The event handling is implemented in an asynchronous way so that it conforms to the original requirement mentioned on [$150] Automatic status updates #54, and, the logic is still decoupled in the meantime.
  • The logic at src/app-routes.js and inside function ResourceBookingService.updateResourceBooking are slightly altered
    to work with the event handling. Please review these changes particularly to ensure nothing would be broken due to that.

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imcaizheng, automatic updates doesn't work for me. When I make any action which supposes to trigger automatic updates, I get error.

  1. When I update ResourceBooking status to assigned I get an error:

    image

    • To reproduce I create Job, JobCandidate and ResourceBooking using the same userId, to trigger automatic status update logic.
  2. If I change the status of Job to cancelled I get an error:

    image

@imcaizheng imcaizheng requested a review from maxceem December 26, 2020 08:18
@imcaizheng
Copy link
Contributor Author

@maxceem I tried to resolve the conflict between the PR and dev branch but I did it wrongly. Please discard the last commit and merge the last but one, named revert the change made to app-routes.js.

@maxceem
Copy link
Contributor

maxceem commented Dec 26, 2020

@imcaizheng hmm, I don't think there is a way to discard the last commit during merging. Could you please, revert the wrong commit from imcaizheng:implement-event-handlers branch, so it's not included in the PR?

This reverts commit fdf0571, reversing
changes made to 40b1f93.
@imcaizheng
Copy link
Contributor Author

@maxceem Thanks. The change is reverted.

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxceem Thanks. The change is reverted.

@imcaizheng Looks like during merging changes that were done for permissions in this PR #77 were reverted.

image

I try to test functionality anyway:

  • When I assign Resouce Booking the corresponding JobCandidate became 'selected'.
  • But if I cancel Job, corresponding Resource Booking and JobCandidates are not cancelled.

@imcaizheng
Copy link
Contributor Author

imcaizheng commented Dec 28, 2020

@maxceem
Please accept this PR topcoder-platform/taas-es-processor#5 for taas-es-processor and try again. Resource Booking and JobCandidates could be properly cancelled after that.

EDIT
Please continue with #85. The merge conflict is resolved there.

@imcaizheng imcaizheng closed this Dec 28, 2020
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