WikiEducationFoundation / WikiEduDashboard

Wiki Education Foundation's Wikipedia course dashboard system
https://dashboard.wikiedu.org
MIT License
392 stars 631 forks source link

Feature: Allows Programs to be Deleted before Removing Campaigns on Programs & Events Dashboard #6012 #6015

Closed empty-codes closed 2 weeks ago

empty-codes commented 3 weeks ago

What this PR does

Solves Issue #6012 and https://phabricator.wikimedia.org/T378013 by removing the requirement to remove campaigns before deleting a program is the behavior on Programs & Events Dashboard.

Changes Made:

  1. When a user attempts to delete a program with the 'Delete Program' button: -If in the Wiki Ed environment or if the program has no associated campaigns, it is deleted immediately using the normal deleteCourse actions. -If in a non-Wiki Ed environment i.e on Program and Events Dashboard and the program is linked to campaigns, it removes the program from the associated campaign(s) before deletion, using new removeAndDeleteCourse actions.

This makes deleting courses more flexible by eliminating the previous need to first remove all campaign associations manually. It however retains the requirement to remove campaigns before deleting a course if in wiki_education mode.

  1. Alerts and hover-activated titles showing course delete instructions removed on the Program & Events Dashboard

Screenshots:

Before: Could not delete programs that were associated with campaigns, no DeleteCourseWorker jobs seen in sidekiq

before alert, tooltip

After: Can now delete programs that are associated with campaigns, DeleteCourseWorker jobs seen in sidekiq and sentry

after working now

Evidence of Deletion in Sidekiq: world reg cropped

Evidence of Deletion in Sentry: image

empty-codes commented 3 weeks ago

@ragesoss What I have done currently is to remove the alert and also stop the instructions from showing when you hover over the delete button on the Programs & Events Dashboard.

I want to confirm if I am to change the logic of the 'Delete program' button itself or if this is enough.

If the logic of the 'Delete program' button on the course page is to be changed, should I change it to match the logic of the 'Remove and Delete' button in the campaigns view? So when you try to delete the program before removing the campaigns, it automatically removes campaigns if any and then deletes the program.

image vs. image

Current 'Delete program' button logic:

in available_actions.jsx

// If course is not published, show the 'delete' button to instructors and admins.
if ((user.isAdvancedRole || user.admin) && (!course.published || !Features.wikiEd)) {
  controls.push((
    <div title={Features.wikiEd ? I18n.t('courses.delete_course_instructions') : undefined} key="delete" className="available-action">
      <button className="button danger" onClick={deleteCourseFunc}>
        {CourseUtils.i18n('delete_course', course.string_prefix)}
      </button>
    </div>
  ));
}

const deleteCourseFunc = () => {
  const enteredTitle = prompt(I18n.t('courses.confirm_course_deletion', { title: course.title }));
  // Check if enteredTitle is not null before calling trim.
  if (enteredTitle !== null && enteredTitle.trim() === course.title.trim()) {
    return dispatch(deleteCourse(course.slug));
  } else if (enteredTitle) {
    return alert(I18n.t('courses.confirm_course_deletion_failed', { title: enteredTitle }));
  }
};

in course_actions.js

export const deleteCourse = (courseSlug) => (dispatch) => {
  return API.deleteCourse(courseSlug)
    .then(data => dispatch({ type: 'DELETED_COURSE', data }))
    .catch(data => dispatch({ type: API_FAIL, data }));
};

in api.js

async deleteCourse(courseId) {
  const response = await request(`/courses/${courseId}.json`, {
    method: 'DELETE'
  });
  if (!response.ok) {
    logErrorMessage(response);
    const data = await response.text();
    response.responseText = data;
    throw response;
  }
  window.location = '/';
  return response.json();
}

'Remove and Delete' Button Logic:

in course.js

const deleteCourseBtn = document.getElementsByClassName('delete-course-from-campaign')[0];
if (deleteCourseBtn) {
  deleteCourseBtn.addEventListener('click', (e) => {
    const enteredTitle = window.prompt(I18n.t('courses.confirm_course_deletion', { title: e.target.dataset.title }));
    if (!enteredTitle) {
      e.preventDefault();
    } else if (enteredTitle.trim() !== e.target.dataset.title.trim()) {
      e.preventDefault();
      alert(I18n.t('courses.confirm_course_deletion_failed', { title: enteredTitle }));
    }
  });
}

in app/views/campaigns/_course_row.html.haml:

- if @presenter&.can_delete_course?
  %a.course-link{:href => "#{course_slug_path(course.slug)}", style: "padding-top: 0;"}
    = form_for(@campaign, url: "/courses/#{course.slug}.json/delete_from_campaign?campaign_title=#{@campaign.title}&campaign_id=#{@campaign.id}&campaign_slug=#{@campaign.slug}", method: :delete, id: "delete_course-#{course.id}", html: { class: 'delete-program-form' }) do
      = hidden_field_tag('course_title', course.title, id: "course_title-#{course.id}")
      %button.button.danger.delete-course-from-campaign{'data-id' => course.id, 'data-title' => course.title, 'data-campaign-title' => @campaign.title, title: t('campaign.delete_course_tooltip') }
        = t('assignments.delete')

in routes.rb:

delete 'courses/:slug/delete_from_campaign' => 'courses/delete_from_campaign#delete_course_from_campaign', as: 'delete_from_campaign',
  constraints: { slug: /.*/ }

in [delete_from_campaign_controller](https://github.com/WikiEducationFoundation/WikiEduDashboard/blob/bac12da90547405e3d6a0c2cfdaaf105c45e9fa0/app/controllers/courses/delete_from_campaign_controller.rb#L6):

def delete_course_from_campaign
  validate
  if @course.campaigns.size > 1
    remove_course_from_campaign_but_not_deleted
  else
    remove_and_delete_course_from_campaign
  end
end

def remove_and_delete_course_from_campaign
  campaigns_course = find_campaigns_course
  result = campaigns_course.destroy
  message = result ? 'campaign.course_removed_and_deleted' : 'campaign.course_already_removed'
  flash[:notice] = t(message, title: @course.title, campaign_title: params[:campaign_title])
  DeleteCourseWorker.schedule_deletion(course: @course, current_user:)
  redirect_to_campaign_path
end

def remove_course_from_campaign_but_not_deleted
  campaigns_course = find_campaigns_course
  result = campaigns_course.destroy
  message = result ? 'campaign.course_removed_but_not_deleted' : 'campaign.course_already_removed'
  flash[:notice] = t(message, title: @course.title, campaign_title: params[:campaign_title])
  redirect_to_campaign_path
end

refs: https://github.com/WikiEducationFoundation/WikiEduDashboard/pull/5583 Thank you!

ragesoss commented 3 weeks ago

Yes, I think the right way is to change the logic to work the same way as the 'remove and delete' button on the campaign page: it should first remove the campaigns and then delete the course.