Fixed Bug418 #58

Merged
mrlan merged 7 commits from Bug418-YAAQOB-MPIANA into Hui-Organize 2024-01-08 18:24:22 +08:00
Collaborator
Bug details can be found here: http://118.25.96.118/kanboard/?controller=TaskViewController&action=show&task_id=2738&project_id=170
yaaqob self-assigned this 2023-11-27 10:44:30 +08:00
yaaqob added 1 commit 2023-11-27 10:44:30 +08:00
mrlan reviewed 2023-11-27 12:53:48 +08:00
Script.php Outdated
@ -919,2 +918,3 @@
$sql = "INSERT INTO `course_ta`(`Course_ID`, `TA`) VALUES ('$id','$ta')";
// Check if the TA is already assigned to the course
$check_sql = "SELECT * FROM `course_ta` WHERE `Course_ID`='$id' AND `TA`='$ta'";

@yaaqob

Thanks. Why is the punctuation mark ` in `course_ta` needed?

@yaaqob Thanks. Why is the punctuation mark \` in \`course_ta\` needed?
Poster
Collaborator

@mrlan
I'm not sure myself, my teammate MPIANA wrote this part. However, it is working fine.

@mrlan I'm not sure myself, my teammate MPIANA wrote this part. However, it is working fine.
yaaqob added 1 commit 2023-12-04 12:31:07 +08:00
Poster
Collaborator

@mrlan
I made a new commit that edits MPIANA's code which is to remove the punctuation ( ` ) from the SQL statement as you suggested him to do, and I merged MPIANA's test cases with this branch.

Also MPIANA asked me to leave this answer to you here regarding why he added the punctuation:

"Backticks in SQL queries, like in the PHP code, ensure cross-database compatibility. They're a good practice for portability across different database systems, promoting coding standards."

@mrlan I made a new commit that edits MPIANA's code which is to remove the punctuation ( ` ) from the SQL statement as you suggested him to do, and I merged MPIANA's test cases with this branch. Also MPIANA asked me to leave this answer to you here regarding why he added the punctuation: "Backticks in SQL queries, like in the PHP code, ensure cross-database compatibility. They're a good practice for portability across different database systems, promoting coding standards."
mrlan reviewed 2023-12-04 17:44:20 +08:00
@ -0,0 +9,4 @@
driver = webdriver.Chrome()
# Open the admin url
driver.get("http://localhost/itech/lrr/Admin.php")

@yaaqob

Please move this statement to the function body of assign_ta_test().

@yaaqob Please move this statement to the function body of `assign_ta_test()`.
mrlan reviewed 2023-12-04 17:45:30 +08:00
@ -0,0 +48,4 @@
tas = ["MPIANA", "KABWANGA", "mark"]
@pytest.mark.parametrize("course_name, ta_name", [(course, ta) for course in courses for ta in tas])
def assign_ta_test(course_name, ta_name):

@yaaqob

Please rename this function to test_assign_TAs().

@yaaqob Please rename this function to `test_assign_TAs()`.
mrlan reviewed 2023-12-04 17:47:34 +08:00
@ -0,0 +62,4 @@
result_file.write(f"Course={course_name}, TA={ta_name}, Result={result}, Alert={alert_text}\n")
# Close the browser window
driver.quit()

@yaaqob

Are you sure driver.quit() will be executed if this script is run with this command: pytest assign_ta_test.py?

@yaaqob Are you sure `driver.quit()` will be executed if this script is run with this command: `pytest assign_ta_test.py`?
mrlan reviewed 2023-12-04 17:49:54 +08:00
@ -0,0 +52,4 @@
alert_text = assign_ta(driver, course_name, ta_name)
try:
assert "Success" in alert_text or "Error" in alert_text

@yaaqob

Thanks

Why do you use OR logic to combine two different things: assert "Success" in alert_text or "Error" in alert_text?

I believe these two cases should be separated.

@yaaqob Thanks Why do you use OR logic to combine two different things: `assert "Success" in alert_text or "Error" in alert_text`? I believe these two cases should be separated.
mrlan reviewed 2023-12-04 17:51:55 +08:00
@ -0,0 +51,4 @@
def assign_ta_test(course_name, ta_name):
alert_text = assign_ta(driver, course_name, ta_name)
try:

@yaaqob

I think the lines from 54-62 can be replaced with a single assert statement.
I don't see try ... except block in pytest test scripts quite often.
If the assertion fails, pytest will tell us.

@yaaqob I think the lines from 54-62 can be replaced with a single assert statement. I don't see `try ... except` block in pytest test scripts quite often. If the assertion fails, pytest will tell us.
mrlan reviewed 2023-12-04 17:56:10 +08:00
@ -0,0 +47,4 @@
courses = ["Python", "computer", "testing"]
tas = ["MPIANA", "KABWANGA", "mark"]
@pytest.mark.parametrize("course_name, ta_name", [(course, ta) for course in courses for ta in tas])

@yaaqob

Does this test the scenario that a TA is assigned to the same course twice?

[(course, ta) for course in courses for ta in tas]

[('Python', 'MPIANA'), ('Python', 'KABWANGA'), ('Python', 'mark'), 
('computer', 'MPIANA'), ('computer', 'KABWANGA'), ('computer', 'mark'), 
('testing', 'MPIANA'), ('testing', 'KABWANGA'), ('testing', 'mark')]
@yaaqob Does this test the scenario that a TA is assigned to the same course twice? ``` [(course, ta) for course in courses for ta in tas] [('Python', 'MPIANA'), ('Python', 'KABWANGA'), ('Python', 'mark'), ('computer', 'MPIANA'), ('computer', 'KABWANGA'), ('computer', 'mark'), ('testing', 'MPIANA'), ('testing', 'KABWANGA'), ('testing', 'mark')] ```

@yaaqob

Thanks.

Could you sync your branch with the remote branch Hui-Organize, and then make a pull request that requests to merge to Hui-Organize?

See pull request 59 for one example.

Hui

@yaaqob Thanks. Could you sync your branch with the remote branch Hui-Organize, and then make a pull request that requests to merge to Hui-Organize? See pull request 59 for one example. Hui
mrlan added 153 commits 2023-12-23 15:55:07 +08:00
mrlan changed title from Fixed Bug418 to Fixed Bug418 2023-12-23 18:26:14 +08:00
mrlan changed target branch from master to Hui-Organize 2023-12-23 18:26:15 +08:00
yaaqob added 3 commits 2023-12-27 00:12:05 +08:00
mrlan merged commit 70d2bb4504 into Hui-Organize 2024-01-08 18:24:22 +08:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: mrlan/LRR#58
There is no content yet.