System for discouraging assignment of specific articles #5495

Abishekcs · 2023-10-04T07:13:36Z

What this PR does

This pull request addresses the issue documented in #5477. It introduces a feature that displays a warning message to students and administrators when they select a discouraged article/assignment.

Affected Pages:

/courses/[institution_name]/[course_name]/articles/available
/courses/[institution_name]/[course_name]/article_finder
/courses/[institution_name]/[course_name]/students/articles/[user]
/courses/[institution_name]/[course_name/home

Screenshots

Before:

For Admin :

Before.Admin.webm

For Student :

Before.Student.webm

After:

For Admin:

After.Admin.mp4

For Student:

After.Student.mp4

Note:

For testing a category from wikipedia set blocked_assignment_category: [Category Name] in application.yml (used in assignment_manager.rb) and also run dailyupdate

app/assets/javascripts/components/article_finder/article_finder_row.jsx

app/assets/javascripts/components/articles/add_available_articles.jsx

app/assets/javascripts/components/articles/available_article.jsx

pppery

I see nothing obviously wrong with this code, but I'm not an expert here.

Abishekcs · 2023-10-21T03:59:08Z

I see nothing obviously wrong with this code, but I'm not an expert here.

okay np.

Abishekcs · 2023-10-21T04:06:34Z

@ragesoss, your review would be greatly appreciated whenever you have the time.

ragesoss · 2023-10-23T20:46:29Z

Thanks for taking on such a big issue @Abishekcs!

I think I would prefer a somewhat different approach. The first major suggestion I have is to rely on the existing Category table rather than create a new record type in the database. These records are already designed to query Wikipedia for the titles in a category and save them, so I don't think there will be a need for any database changes.

I also think that the majority of the logic for this feature should be on the server. AssignmentManager is the natural place to handle checking for whether a new assignment is one that ought to be blocked or requires a warning confirmation.

I suggest breaking it up into two parts, beginning with implementing a system that just completely prevents creating assignments that are in a certain category. This would be relatively easy to handle in terms of data flow: AssignmentManager should check against the category, raise a specific error (similar to the existing DuplicateAssignmentError), and then AssignmentsController should rescue that error and return a good error message and error status to the frontend. (The Category will need to be routinely refreshed, which can be done via a worker that gets scheduled via DailyUpdate.)

A follow-up could then extend it to handle articles that are discouraged but not prevented altogether. We could do this either by adding a way to acknowledge the warning and resubmit from the frontend, or by triggering an Alert when a discouraged articles is assigned and then sending an email to the user and/or the instructor with details about discouraged articles and why they should consider choosing a different article instead. Hmmm... after thinking through it, I guess the frontend approach will be better there, because one goal would be to prevent the noise of adding additional templates to the talk page of the article (even if they later get removed).

Abishekcs · 2023-10-24T10:12:32Z

Sure, I will make the changes you have suggested. Thank you for the review .

app/assets/javascripts/components/common/AssignCell/AssignButton.jsx

app/assets/javascripts/utils/api.js

app/models/wikipedia_category_member.rb

app/assets/javascripts/components/articles/add_available_articles.jsx

Abishekcs · 2023-11-15T19:14:40Z

@ragesoss , I have moved the logic for discouraged assignments to AssignmentManager and am now utilizing the existing category. I would appreciate a review whenever you have the time.(Updated the Screenshots)

app/assets/javascripts/utils/api.js

lib/assignment_manager.rb

lib/wiki_discouraged_article_manager.rb

Abishekcs · 2023-11-22T19:02:50Z

@ragesoss, I have removed the warning feature for this PR and kept the feature for blocked assignment. Currently, I am writing test code for this PR.

ragesoss · 2023-11-22T19:06:26Z

@Abishekcs sounds good. Give me a ping when the tests are done. I will likely have some time to review it next week if it's ready then.

Abishekcs · 2023-11-27T14:35:33Z

@ragesoss the test code is done for this PR. I would appreciate a review whenever you have the time.

ragesoss · 2023-11-27T22:52:33Z

it's on my todo list.

app/controllers/assignments_controller.rb

lib/data_cycle/daily_update.rb

lib/wiki_discouraged_article_manager.rb

…eate_assignment Move the check against discouraged articles into the create_assignment method, parallel to the existing pattern for handling DuplicateAssignmentError. This simplifies the code structure, ensures the same clean title flow is applied, and maintains consistency in error handling.

…eManager Replace CategoryImporter with #refresh_titles, ensuring uniqueness with find_or_create_by.

Refactor and enable Wiki Education-only features for update_wiki_discouraged_article.

Abishekcs · 2023-12-05T19:31:17Z

I fixed it by updated the selenium-webdriver gem, and I've pushed that update. If you rebase and bundle install, hopefully the tests will work properly for you now.

@ragesoss the test is working properly now.
For this, Failed examples:
rspec ./spec/lib/importers/revision_score_importer_spec.rb:205 # RevisionScoreImporter.update_revision_scores_for_all_wikis imports data and calcuates an article completeness score for available wikis its working fine locally.

ragesoss · 2023-12-05T19:41:27Z

Great. That failure is unrelated to your PR, so the tests seem to be in order. I will continue trying out and reviewing it soon. Thanks!

ragesoss · 2023-12-11T20:36:10Z

config/locales/en.yml

@@ -265,6 +265,7 @@ en:
    article: The article %{title} is now assigned to you.
    article_link: Article
    article_link_wikidata: Item
+    discouraged_article: The article is discouraged by Wiki Education.


This isn't actually being used, because the message is being set already where the error is raised. I suggest using the interpolation syntax.

I suggest the key blocked_assignment, and a message like "%{title} could not be assigned because it is on the Wikipedia community's Wiki Education assignment blocklist."

Okay I will make the changes

ragesoss · 2023-12-11T20:38:46Z

lib/assignment_manager.rb

@@ -104,6 +105,14 @@ def set_article_from_database
    @article = exact_title_matches.first
  end

+  def check_wiki_edu_discouraged_article
+    category = Category.where('name = ? AND article_titles LIKE ?',


I think the better way to do this is to find the category first, and then check whether the title is included in the catetgory's array of titles. Using LIKE might result in mismatches, such as blocking the assignment of "Animal" when "Animal rights" is on the blocklist.

ragesoss · 2023-12-11T20:39:53Z

I tested this out, and it works the way I would expect. With the additional changes I requested, this will be ready to merge.

…rror message and enhance blocklist check Refactors error messaging to use interpolation syntax and introduces 'blocked_assignment' key. Improves blocklist check by finding the category first, ensuring accurate determinations and avoiding potential mismatches.

Abishekcs · 2023-12-16T00:23:27Z

I tested this out, and it works the way I would expect. With the additional changes I requested, this will be ready to merge.

@ragesoss , I have made the requested changes for this.

Abishekcs · 2024-01-05T07:41:51Z

@ragesoss any update on this PR😅 ?

ragesoss · 2024-01-05T16:29:07Z

@Abishekcs yes, i'm happy with the PR and it is ready to merge, but I have not yet introduced this new feature to the Wikipedia community or finalized the category name to so, so I don't want to merge and deploy it yet. Doing that introduction is near the top of my todo list now that I am back from vacation; I hope to get it started today.

ragesoss · 2024-02-15T18:47:58Z

The on-wiki discussion about this has not really come to a conclusion, in particular with regards to the category name or how the community wants to use it... but I'd like to go ahead and merge it and deploy it so that it's ready whenever we do come to consensus. So... what happens if this gets deployed but there is no blocked_assignment_category value set?

Abishekcs · 2024-02-15T19:07:53Z

@ragesoss by this So... what happens if this gets deployed but there is no blocked_assignment_category value set? do you mean if there is no ENV set for blocked_assignment_category 😅?

ragesoss · 2024-02-15T19:37:42Z

Yes, that's what I mean. Is it safe to deploy it without such a value, so that it just would not do anything? If not, can it be easily adjusted to behave like that?

Abishekcs · 2024-02-15T19:49:17Z

@ragesoss, ENV['blocked_assignment_category'] is used in two places:

a) In AssignmentManager, it is used to find a category by name. It simply returns null if there is no value set for ENV['blocked_assignment_category'] when I test it locally.

b) The second place is in WikiDiscouragedArticleManager, which is used in dailyupdate. I think for the second part, I will add an if statement to check if ENV is set. Since it might cause trouble with other daily updates.

In WikiDiscouragedArticleManager, introduce a conditional check to verify if 'blocked_assignment_category' is present in the environment variables. When absent, log an informational message using Rails.logger.

Abishekcs · 2024-02-15T20:18:42Z

Ignore the check ENV it was just for testing locally if it gives error.

@ragesoss i have added the if statement for WikiDiscouragedArticleManager and for AssignmentManager there is already an if statement to check category is present or not.

ragesoss · 2024-02-15T20:24:27Z

Great, thanks!

ragesoss · 2024-02-15T20:25:37Z

lib/wiki_discouraged_article_manager.rb

-      depth: 0
-    ).refresh_titles
+    blocked_assignment_category = ENV['blocked_assignment_category']
+


I'd prefer a return unless guard clause here, rather than the branching if statement.

ragesoss · 2024-02-15T21:36:09Z

Looks like your new change broke the tests. The fix probably involves adding a new entry in config/environments/test.rb.

Abishekcs · 2024-02-15T21:50:44Z

Looks like your new change broke the tests. The fix probably involves adding a new entry in config/environments/test.rb.

It should pass now 😅 added the entry for test ENV.

ragesoss · 2024-02-26T21:48:28Z

Thanks @Abishekcs! This is an exciting feature, and I'm looking forward to seeing it in action.

Abishekcs · 2024-02-27T02:47:18Z

Thanks @Abishekcs! This is an exciting feature, and I'm looking forward to seeing it in action.

Great! 👍

pppery reviewed Oct 5, 2023

View reviewed changes

Abishekcs changed the title ~~System for discouraging assignment of specific articles [WIP]~~ System for discouraging assignment of specific articles Oct 20, 2023

Abishekcs marked this pull request as ready for review October 20, 2023 10:05

Abishekcs requested a review from pppery October 20, 2023 10:33

pppery reviewed Oct 20, 2023

View reviewed changes