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

Add draft flag to activities #4998

Merged
merged 35 commits into from
Oct 26, 2023
Merged

Add draft flag to activities #4998

merged 35 commits into from
Oct 26, 2023

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Sep 22, 2023

This pull request introduces the concept of draft activities.

What is a draft activity?

Every newly added activity is considered a draft activity until they are published.

The idea is that an activity stays in draft mode until its creator greenlights it manually using the Dodona UI.

What is the effect of a draft activity?

The concept of draft activities serves several goals.

Prevent clutter

Draft activities are only visible for repository owners and course admins for courses with the activity. This prevents that multiple copies of our example exercises are present in the global database for everyone to see.

Reduce false-positive error messages

The Dodona admins are notified of severe errors (internal error) during the execution of a submission. When a teacher is creating a new exercise and experimenting with adding tests, these errors are quite common. Because of this, the Dodona admins often ignore these messages, causing real issues to be unnoticed. When an exercise is still in draft mode, we will no longer notify the Dodona admins.

Improve discoverability

When you add a new exercise to Dodona, it is not always easy to find that exercise and try it yourself. We will now list all draft exercises of a user on the home page, making these easier to find.

Future additions

This is just a first set of changes. In the future, we might add additional features for draft activities:

  • If a draft activity is removed from the repository, we may automatically remove it from Dodona instead of just marking it as removed.
  • If a teacher marks a draft as finalized, we may explicitly ask if they want to make the exercise public or private. This can further reduce the number of (unwanted) public exercises.
  • Provide extra debug information. If the judge crashes due to an internal error, some debug information is logged for the Dodona admins. We may also show this information to teachers if an exercise is in draft mode.

image
image
In the below layout, the activity status icon is also moved to avoid a to long list of symbols
image
image
image
image
image

Closes #4845

@jorg-vr jorg-vr added the feature New feature or request label Sep 22, 2023
@jorg-vr jorg-vr self-assigned this Sep 22, 2023
@jorg-vr jorg-vr marked this pull request as ready for review September 26, 2023 12:43
@jorg-vr jorg-vr requested a review from a team as a code owner September 26, 2023 12:43
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team September 26, 2023 12:43
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

One small edge case I noticed:

  • When an existing exercise is changed to draft, the exercise still seems to appear on the home page of students who have submitted to this exercise (although if you click on it, it correctly says you don't have access). Not sure if this needs fixing.

app/views/pages/_user_card.html.erb Outdated Show resolved Hide resolved
config/locales/views/activities/nl.yml Outdated Show resolved Hide resolved
jorg-vr and others added 2 commits September 27, 2023 11:42
Co-authored-by: Niko Strijbol <strijbol.niko@gmail.com>
Co-authored-by: Niko Strijbol <strijbol.niko@gmail.com>
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Sep 27, 2023

The activity is not directly linked if it is not accessible in the recent activities:
image

On some other places (such as submission detail), the link to the activity can still be found, But gives a correct error message.

This behavior is similar to that of series without access our private exercises etc, I would not change it.

I guess it also won't happen often that an existing activity is set to draft after students used it.

@jorg-vr jorg-vr requested a review from niknetniko September 27, 2023 12:03
@pdawyndt
Copy link
Contributor

Maybe it could help if students also see the draft status, which is in contrast with the private status that is completely transparent for students. At least, having the draft status visible for students would explain for them why clicking the exercise results in a "no access", and then students could remind their teachers that exercises are still in draft mode.

Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

I don't immediately have better suggestions, so more as a mental note, but the "draft mode" and especially "ontwerpmodus" terminology feel a bit weird to me.

@pdawyndt
Copy link
Contributor

@niknetniko: would the mental model feel more natural if it were branded as published/unpublished?

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

I've been thinking about this some more. I there a good reason why someone might convert an existing exercise back into draft? Right now, doing so might cause problems if the exercise is used in other courses.

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Oct 20, 2023

When updating an exercise this might be usefull. Eg adding more tests: there might be mistakes in those new tests so while working on them it can be logical to make the exercise temporarily unavailable for students

Teachers of these other course will still have access and see the draft notice

app/models/activity.rb Outdated Show resolved Hide resolved
app/views/activities/_form.html.erb Show resolved Hide resolved
app/models/activity.rb Outdated Show resolved Hide resolved
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

As discussed during the day: if we make draft a one-way setting, we can just store it in the database instead of also in the config file. This would simplify the code a bit.

  • The default value in the database can be set to true so we can remove the before_create action and the other logic to add it to the config file and set it for new exercises. We could also set it on creation in the tests and prevent a manual update. The migration would probably be something like
add_column :activities, :draft, :boolean, default: true
Activity.reset_column_information # don't know if this is strictly needed
Activity.update_all(draft: false)
  • The setting can be removed from the form. I would add the alert that is now shown on the show page to the edit and info page as well. publish could be added as a separate action to the controller (which would prevent adding a new check that only allows it to be set from true to false)

@jorg-vr jorg-vr marked this pull request as draft October 26, 2023 08:29
@jorg-vr jorg-vr marked this pull request as ready for review October 26, 2023 09:58
@jorg-vr jorg-vr requested review from bmesuere and chvp October 26, 2023 09:58
@jorg-vr jorg-vr marked this pull request as draft October 26, 2023 09:59
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Oct 26, 2023

Still have to add the alert to other pages, will fix.

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Oct 26, 2023

The migration would probably be something like

I took a slightly different approach: first create column with default false, then change the default to true
This results in the default only being true for new exercises

publish could be added as a separate action to the controller

I prefer the current approach with an extra validation in activity. This keeps our api more default and more RESTfull

@jorg-vr jorg-vr marked this pull request as ready for review October 26, 2023 11:35
@niknetniko niknetniko self-requested a review October 26, 2023 11:39
@@ -56,6 +57,7 @@ class Activity < ApplicationRecord
has_many :labels, through: :activity_labels

validates :path, uniqueness: { scope: :repository_id, case_sensitive: false }, allow_nil: true
validates :draft, inclusion: { in: [false] }, unless: :draft_was
Copy link
Member

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if not draft_was, validate if draft is in the list of possible values, with the only possible value being false
draft_was is the previous value of draft

so if the previous value of draft was false, only allow false as new value

Copy link
Member

Choose a reason for hiding this comment

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

You could argument that this check should be in the controller instead of the model, but not a strong opinion so ok for me.

@@ -75,7 +75,7 @@ def read?

def permitted_attributes
if update?
%i[access name_nl name_en]
%i[access name_nl name_en draft]
Copy link
Member

Choose a reason for hiding this comment

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

You could also check here that draft can only be updated if it's currently true. Might be a more clear than the validates in the model.

@jorg-vr jorg-vr merged commit 0e77184 into main Oct 26, 2023
13 checks passed
@jorg-vr jorg-vr deleted the feat/add-draft-flag branch October 26, 2023 14:08
@jorg-vr jorg-vr temporarily deployed to naos October 26, 2023 14:09 — with GitHub Actions Inactive
@jorg-vr jorg-vr temporarily deployed to production October 26, 2023 14:13 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add draft state for activities
5 participants