fix:restored removed article to available articles

[shishiro26]

NOTE: Please review the pull request process before opening your first PR: https://github.com/WikiEducationFoundation/WikiEduDashboard/blob/master/CONTRIBUTING.md#pull-request-process

What this PR does

This PR restores both self-assigned and admin-assigned articles back to the pool of available articles.
I have written a test case for the controller to ensure that the changes work as expected.

Screenshots

After:
https://github.com/user-attachments/assets/db77ecb3-734d-4693-94d2-cfd235297c90

Open questions and concerns

With this change, when removing an article, a dialog appears with the message: "Are you sure you want to delete this program?"

• Do we need to modify the dialog message to reflect the new functionality?

#5549

[shishiro26]

In this PR, I have added Ruby test cases for the unclaim method in the AssignmentsController.

The scenarios covered are as follows:

1. When the article is assigned to the user by the admin:
   • The admin removes the assignment.
   • https://github.com/user-attachments/assets/7bbd0d4e-bac7-4f56-9066-af09b595dc84
2. When the article is assigned to the user by the admin:
   • The user removes the assignment.
   • https://github.com/user-attachments/assets/392911db-789f-44dc-81c7-84f73232d7a6
3. When the article is self-assigned:
   • Either the user or the admin removes the self-assigned article.
   • https://github.com/user-attachments/assets/ea029f97-58a5-455c-996d-4d46ee9ee9a2

[shishiro26]

@ragesoss the test is passing locally in my system

[shishiro26]

@ragesoss, I'm not entirely sure, but the multiwiki_assignment_spec.rb test seems to be failing after the latest commit merge. Additionally, the account_requests_spec.rb test is failing both on my local setup and in the master branch. However, this failure appears to be unrelated to the changes I made.

[ragesoss]

@shishiro26 yes, i'm not sure why it started failing, but i'm going to look into it.

[ragesoss]

Does this implementation differentiate articles that started as Available Articles versus ones that the student chose via title without starting as an Available Article? I think we need this to only apply for ones that started as Available Articles.

[shishiro26]

I’m not sure if you’re referring to this one, sir

User:
https://github.com/user-attachments/assets/0cfa08ac-d75f-4f52-a223-a3c63a471ec9

If so, then yes, it doesn’t differentiate between the two. It removes both from the assigned ones and keeps them in the available articles, sir.

[shishiro26]

Sir, I would like to know if there is any user-specific association, such as a user_id or something similar, that tracks whether a particular person created or added an article. Is there an existing reference to identify the user who created or added the article? From the logs I’ve seen, there are references to users, but they seem to be related to assignments rather than article creation. If such an association doesn’t currently exist, would it be possible for me to add a user_id field to the articles table and link it to the user who created or added the article?

[ragesoss]

I think you could use Assignment#flags to keep track of whether an Assignment record began as an Available Article.

[shishiro26]

Sir, I believe there is no change in the flags field, as it remains NULL in both cases in the SQL database. There appears to be no update to Assignment.flags.

[ragesoss]

Right, you would need to implement that.

[shishiro26]

okay, sir! 😄

[shishiro26]

Could you please review this, sir? This is just a rough implementation. If this method looks fine, the unclaim method (which removes the article if it is not in the available articles pool, and if it is in the pool, the assignment is simply unclaimed and added back to the available articles), and if the assignment#flags are set correctly, I will refine the code and update the RSpec tests for the final version.

[ragesoss]

I will plan to review this next week.

[shishiro26]

Okay sir

[ragesoss]

This looks like it's going in the right direction, but from my reading of the code, I think it doesn't correctly the initial setting of flags. An "Available Articles" is an Assignment record created by the instructor of a course (or an admin) that is not linked to a user. These are the ones that are available as a list for students to select as their assignment. In contrast, a newly created assignment that is linked to a user, which happens when a student enters an article title directly rather than selecting from the Available Articles list, does not start as an Available Article. Those two situations are what the flag should keep track of: whether a new Assignment started as Available Article or not.

I think feature specs should be extended to test for the new behavior. One should test for the presence of the flag when an instructor adds an Available Article, and the absence when a student adds an assigned article directly. Another should test for a student claiming and then unclaiming an assignment, verifying that the article returns to the Available list. These might be possible to test via adjusting or extending existing tests that interact with assignment features, such as my_articles_spec.rb and instructor_role_spec.rb

[shishiro26]

Thank you for the clarification. I realized I had overlooked this situation earlier. When adding assignments, I mistakenly assumed they were directly linked to articles rather than being part of the assignments workflow. Upon reviewing, I noticed that when adding an available_article, the user_id is set to nil and it gets added to the assignments. I’ll address this issue and ensure the flags are set correctly to distinguish between "Available Articles" and directly assigned articles.

Additionally, I’ll review and update the tests to align with the expected behavior. I now have a clearer understanding of the logic, but I’ll double-check the code and adjust or extend the relevant tests before committing the changes.

[shishiro26]

• When a user tries to assign themselves an available article and then unclaims it:
  https://github.com/user-attachments/assets/eaffb8cf-0cc5-46e7-8942-d0109021609c

• When a user tries to assign themselves an unavailable article and then unclaims it:
  https://github.com/user-attachments/assets/f9827066-5aff-4f80-9a3a-690e2dcb5b4f

• When an admin assigns a user an available article and the user (or admin) unclaims it:
  https://github.com/user-attachments/assets/35ac1113-8658-49c9-9d45-83758ea3c40e

• When a user tries to assign an unavailable article to another user:
  https://github.com/user-attachments/assets/bcc4857e-8c61-4d90-bbdb-e29b7aa4e9a9

I've also updated the test cases to reflect these scenarios. However, I am unsure if I should update the flags in the create_assignment within the assignment_controller_spec.rb. Could you clarify this?

[ragesoss]

I am unsure if I should update the flags in the create_assignment within the assignment_controller_spec.rb. Could you clarify this?

I'm not sure what you're referring to here; I don't see create_assignment in that spec. However, it would make sense to ensure that spec tests the creation of assignment both with and without a user, and verify the flag is present as expected in the case of creating an assignment without a user. I think that both adding Available Articles and adding user-assigned Assignments both go through the #create method of that controller.

[shishiro26]

I've only added the test for available articles, while the tests for user-assigned assignments are already present and not covered in this test, sir. Also, the assigned_articles_spec.rb was running successfully on my local machine, sir.

[shishiro26]

Could you please take a look at this, @ragesoss?

[ragesoss]

@shishiro26 it's on my todo list, but I likely won't be able to review it until next week. @Abishekcs: if you're interested in having a look, I think your feedback would be helpful in the meantime.

[Abishekcs]

@shishiro26 it's on my todo list, but I likely won't be able to review it until next week. @Abishekcs: if you're interested in having a look, I think your feedback would be helpful in the meantime.

Sure, I will look into it.😄

[Abishekcs]

Open questions and concerns

With this change, when removing an article, a dialog appears with the message: "Are you sure you want to delete this program?"

• Do we need to modify the dialog message to reflect the new functionality?

How about adding a "notification message" after successfully unclaiming an assignment that belonged to available articles?

By dispatching an ADD_NOTIFICATION action in the unclaimAssignment thunk. This way, the one who removes the article will be clearly notified if an article has been returned to the available pool and don't always have to check the if the article was successfully added back to the available articles.

[shishiro26]

I believe this new implementation works well. When an assignment is unclaimed, it won't display any dialog box or confirmation message. Instead, it will directly unclaim the assignment and show a notification.

[Abishekcs]

I believe this new implementation works well. When an assignment is unclaimed, it won't display any dialog box or confirmation message. Instead, it will directly unclaim the assignment and show a notification.

Probably should keep the existing dialog box and confirmation message. Just add the Notification(that is for the article that is removed and belongs to available article section) after the user has confirmed his choice.

[Abishekcs]

Moving the successNotification definition inside the unclaimAssignment function would be better. Since this notification is specifically tied to the success state of unclaiming an article, it makes more sense to define it within the function itself rather than passing it as a parameter.

[shishiro26]

Yes, but I noticed that in other parts of the code, the notification is also being sent as a parameter. So, I kept it that way and did not change it.

[Abishekcs]

Yes, but I noticed that in other parts of the code, the notification is also being sent as a parameter. So, I kept it that way and did not change it.

ohh but still it would be nice to move as much as logic to redux the less parameter to pass better.

[Abishekcs]

Additionally, I am not sure but using assignment to check for successful unclaim may not be the most reliable indicator. Instead, consider using flags or checking if user_id is null/undefined, as these would more explicitly confirm that the article was successfully unclaimed.

[shishiro26]

Yuppp