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

Task change assignee #83

Merged
merged 14 commits into from
Nov 29, 2021
Merged

Conversation

orzionpour
Copy link
Collaborator

@orzionpour orzionpour commented Nov 26, 2021

Fixes #77
This PR adds the ability to change the assigned employee of a given task in DB.

Changes you made

- Create conftest.py for reusable fixtures
Added fixtures that create a test DB we can use in future tests (and refactor previous tests to prevent code duplications that we already have).
- Add change_assignee function to Task model
This function is a Task instance's function that changes the assigned employee for the given task.
- Add change_assignee tests
Tests for change_assignee tests.

- Create conftest.py for reusable fixtures
- Add change_assignee function to Task model.
- Add tests for change_assignee
Asafh7
Asafh7 previously approved these changes Nov 26, 2021
AvivLiberman
AvivLiberman previously approved these changes Nov 27, 2021
@nunnatsa
Copy link
Contributor

nunnatsa commented Nov 27, 2021

@orzionpour - Please add a meaningful description, In a human language - what is the change done in this PR - not which files are added etc. but something that your readers (including your future self) will understand.

Copy link
Contributor

@nunnatsa nunnatsa left a comment

Choose a reason for hiding this comment

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

Added some non-blocking comment. Please consider to fix them.

conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
@orzionpour orzionpour added the dont merge Don't merge this PR label Nov 27, 2021
@orzionpour
Copy link
Collaborator Author

orzionpour commented Nov 27, 2021

@orzionpour - Please add a meaningful description, In a human language - what is the change done in this PR - not which files are added etc. but something that your readers (including your future self) will understand.

@nunnatsa
Done. Please let me know if there is something which I didn't covered or isn't clear yet.

- Removed use of Random to prevent unexpected behavior in tests.
- Changed to named parameters in user creations.
Asafh7
Asafh7 previously approved these changes Nov 27, 2021
- Add new tests to validate the correctness of the function in special cases.
- Add new user case handling: new_assignee is a user but not part of the system (not in DB)
- Add corresponding test
@orzionpour orzionpour dismissed stale reviews from Asafh7 and aradz1997 via e49462b November 27, 2021 22:30
conftest.py Outdated Show resolved Hide resolved
conftest.py Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
avraham1992
avraham1992 previously approved these changes Nov 28, 2021
@orzionpour
Copy link
Collaborator Author

Resolved everything.
Note that there will be conflicts when merging #74 and #83, so please after merging one of them, wait so I will resolve the conflicts (the conflicts are only because I worked on the same model file)

conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
conftest.py Show resolved Hide resolved
tasks/tests/test_change_assignee.py Outdated Show resolved Hide resolved
tasks/tests/test_change_assignee.py Outdated Show resolved Hide resolved
kobihk
kobihk previously approved these changes Nov 28, 2021
Asafh7
Asafh7 previously approved these changes Nov 29, 2021
@orzionpour orzionpour dismissed stale reviews from Asafh7 and kobihk via 784ce82 November 29, 2021 14:41
@orzionpour
Copy link
Collaborator Author

Resolved conflicts with #74 - can be merged now.

@mgold1234 mgold1234 merged commit a7bcecf into redhat-beyond:main Nov 29, 2021
@orzionpour orzionpour deleted the task-change-assignee branch December 1, 2021 10:04
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.

Add change assignee function for Task model
8 participants