-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Conversation
There was a problem hiding this 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.
Co-authored-by: Niko Strijbol <[email protected]>
Co-authored-by: Niko Strijbol <[email protected]>
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. |
There was a problem hiding this 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.
@niknetniko: would the mental model feel more natural if it were branded as published/unpublished? |
There was a problem hiding this 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.
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 |
There was a problem hiding this 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)
Still have to add the alert to other pages, will fix. |
I took a slightly different approach: first create column with default false, then change the default to true
I prefer the current approach with an extra validation in activity. This keeps our api more default and more RESTfull |
app/models/activity.rb
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
app/policies/activity_policy.rb
Outdated
@@ -75,7 +75,7 @@ def read? | |||
|
|||
def permitted_attributes | |||
if update? | |||
%i[access name_nl name_en] | |||
%i[access name_nl name_en draft] |
There was a problem hiding this comment.
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.
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:
In the below layout, the activity status icon is also moved to avoid a to long list of symbols
Closes #4845