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

On assignments allow the instructor to set a day/time for it to be make visible #466

Open
barbarer opened this issue Jan 26, 2024 · 25 comments

Comments

@barbarer
Copy link

Right now I have to go into the instructor page and make my assignments for lecture visible just before or during lecture. It would be nice to have them automatically become visible at set day/time.

@jjjonesj
Copy link

I can take on this issue!

@bnmnetp
Copy link
Member

bnmnetp commented Jun 7, 2024

I'm not sure what the best way to handle this is...

Right now we have a simple switch
If we add a date/time then we can just check that before showing the assignment. But what if you want to hide the assignment again, like after an exam? Just set the visible date to something far in the future?

It would be nice to not have both the switch and the date, and I really don't want to have both a show date and a hide date.

@barbarer @bhoffman0 What is the right flow here that will not confuse people?

@barbarer
Copy link
Author

barbarer commented Jun 7, 2024 via email

@jjjonesj
Copy link

jjjonesj commented Jun 7, 2024

Hypothetically, wouldn't the hide date fall under the due date system? Or would we still have the same issue of having the calendar and switch in that case as well? If we want things to be automatic, the due date system would close the assignment once the specific time passes while the visibility just controls when it is seen, maybe?

@bnmnetp
Copy link
Member

bnmnetp commented Jun 7, 2024

Every instructor has their own tolerance for how late to accept work. So you can't just hide the assignment on the due date or even X days after the due date.

In my case I said I would accept late work until I got around to grading it. So sometimes it was days and sometimes a week or more.

@bnmnetp
Copy link
Member

bnmnetp commented Jun 7, 2024

My feeling is that we eliminate the switch, replacing it on the main panel with a date/time picker for when to make the assignment visible. It should default to the current time.

Then inside the "More Options" panel we add a "Hide After " date / time picker.

The logic in the chooseAssignment and doAssignment endpoints will need to be updated to use the dates if they are there, but fall back on the is_visible switch for old assignments created before we add this.

We don't have a background task to monitor the time to make something visible or invisible we just control what can be seen in the two endpoints we have for choosing and doing an assignment.

@jjjonesj
Copy link

jjjonesj commented Jun 7, 2024

I see now, that makes sense. I'll have to explore the files more for the "More Options" and to better understand eliminating the switch over the weekend. In the meantime, if we get rid of the switch for the new date system, how do we keep it partially intact for old assignments?

@bnmnetp
Copy link
Member

bnmnetp commented Jun 8, 2024

The visibility of an assignment is represented in the model/database by the is_visible column. We will shift this to use visible_on or some other better name to indicate when an assignment should be visible.
There are probably two options:

  1. remove the is_visible column from the model. This would force any existing systems to run an update that would set the visible_on time to utcnow if is_visible is True. If is_visible is False then set visible_on to 10 years from now.
  2. keep the is_visible column around for the old interface, (still in use) but have the new interface just set the date.

Again, both the chooseAssignment and doAssignment endpoints will need to have their logic updated to use the new visible_on and/or hidden_on columns.

@jjjonesj
Copy link

So, where exactly do I start with that process? I think we should go with the second option if we're keeping previous assignments in mind. However, if it would be better for any future growth, it'd be best to go with the first option and just have everything updated. It's just a matter of what would benefit the platform overall and the easiest way of going about it at this point, I think.

@bnmnetp
Copy link
Member

bnmnetp commented Jun 11, 2024

Well,

On the UI side:

  • remove the is_visible switch
  • add a date / time element for the visible_on value
  • add a date / time element in the more options panel for hide_on
  • Make sure all that saves properly for an assignment

On the server side:

  • Update chooseAssignment and doAssignment

@jjjonesj
Copy link

Okay! I'm sure I can do both sides with guidance/instructions! I'll work the server side first since that's what my internship centers. I've been looking through the files and have seen that multiple contain chooseAssignment and doAssignment, which specific one do I need to update and how? Forgive me for asking so many questions, this is my first time working on the backend and being a student developer!

@bnmnetp
Copy link
Member

bnmnetp commented Jun 11, 2024

What you should be looking at is in the student.py file under bases/rsptx/assignment_server_api

Anything under web2py is deprecated, and should not be changed or further developed.

@jjjonesj
Copy link

So, is it in these places that I should create a new variable "visible_on" with if statements to trigger the is_visible point?

Screenshot 2024-06-11 152345
Screenshot 2024-06-11 151753
Screenshot 2024-06-11 151809

@jjjonesj
Copy link

Hi, is it okay if we follow up on my previous question since you were absent yesterday? I'm still not quite sure how to update the logic. At first, I had changed the is_visible to visible_on but, I wasn't sure if that was correct and put it back. I was wondering if I needed to on create a new object visible_on using utcnow like the timestamp does.

@bnmnetp
Copy link
Member

bnmnetp commented Jun 13, 2024

Its more than that, the visible_on value needs to be persistent, which means it needs to be stored in the database. So you are going to have to change the model to have a new column to keep track of the visible_on value for each assignment.

Then I think you should probably write a function that takes all the assignment data and returns true if the assignment should be visible and false if not.

It may help to follow the code down a couple of levels to help you understand what is going on.

It may also help you to go through the tutorial here: https://runestone-monorepo.readthedocs.io/en/latest/tutorial.html

@jjjonesj
Copy link

jjjonesj commented Jun 13, 2024

Okay, thank you so much!

@jjjonesj
Copy link

jjjonesj commented Jul 2, 2024

Good morning. I was wondering if you could explain the logic of this if and else state in the crud.py? I know its supposed to show visible assignments if they're marked but, what is vclause doing here? I have successfully added in the hide/visible time objects within assignSlice.js and the assignment builder, so I'm moving down into the .py files. So, I did everything you told and I'm grasping how everything connects. I'm still feeling a little bit all over the place though.
image

@bnmnetp
Copy link
Member

bnmnetp commented Jul 2, 2024

This is how we construct a query dynamically. We want to select assignments for our course. Sometimes we want ALL the assignments and sometimes only the visible assignments. We only want the rows from the table where all three of the clauses in the and_ are true. If we only want the visible assignments then we have to add the conditional. If we want all the assignments then we just make that clause true.

@jjjonesj
Copy link

jjjonesj commented Jul 2, 2024

Thank you for that! So I wanted to know if I'm going down the right path logically. Crud.py uses models.py, student.py and instructor.py use crud.py, correct? If I change the models .visible from boolean to datetime like duedate, how would that affect that dynamic query? I feel like there could be a way to still use that boolean and hook the date system up to it, if that makes sense? Like, would it be possible to still use that dynamic query you have set up but somehow change that if statement to fit a schedule?

I hade originally planned it (in crud.py) this way before asking for help from you:

line 1260 - vis_date=datetime.datetime.utcnow(),
line 1261 - hid_date=datetime.datetime.utcnow(),
line 1262 - curr_date=datetime.datetime.utcnow(),

line 1273 - if curr_date >= vis_date and curr_date < hid_date:
is_visible = True
else:
is_visible = False

I was going off your previous messages here that said to follow this sort of logic in student.py but, if I keep the boolean, is there a way to use that with vclause/dynamic query?

@bnmnetp
Copy link
Member

bnmnetp commented Jul 2, 2024

  1. We really need to keep is_visible as a boolean. There are lots of assignments in the database already and we can't just make a change like that without breaking all of those assignments and making teachers and students unhappy.
  2. Take a look at the fetch_questions_by_search_criteria function in crud.py. There you will see how we build up a list of different clauses and then unpack them all inside the and_ to make the final query.

The vis_date and the hid_date will be stored in the database. You don't want to set them to utcnow. Think about it like this:

The status of an assignment will be in exactly one of the following states:

  1. Upcoming. (not yet visible)
  2. Active.
  3. Active, but late.
  4. Closed. (after the due date, and no longer visible)

All of these states can be inferred by using the current time and comparing that against the vis_date, the hid_date and/or the is_visible flag.

@jjjonesj
Copy link

jjjonesj commented Jul 2, 2024

Okay, I'll work towards that, thank you!

@jjjonesj
Copy link

jjjonesj commented Jul 3, 2024

So, if the vis_date and hid_date shouldn't be set utcnow, can I still keep the datetime.datetime so they're still time objects to be used for comparison? I saw other ones that did this but, their times were preset like (2020, 1, 1, 0, 0, 0). I'm also not sure how to do those conditionals, would they something like this?:

line 1273 - if is_visible:
vclause = Assignment.visible == is_visible
elif curr_date > vis_date:
vclause = Assignment.visible == is_visible
elif curr_date < vis_date:
vclause = Assignment.visible == is_hidden #new boolean in models.py
elif curr_date >= hid_date:
vclause = Assignment.visible == is_hidden
elif curr_date < hid_date:
vclause = Assignment.visible == is_visible
elif is_hidden:
vclause = Assignment.visible == is_visible
else:
vclause = True

So, it compares the dates and has the flags if needed. I looked at the fetch_questions_by_search_criteria like you told me. I saw a parameter that holds the conditions. Criteria, then it's ".author", ".base_course", etc, would I need to set up some variables in a separate file (models maybe?) for this?

@bnmnetp
Copy link
Member

bnmnetp commented Jul 3, 2024

No, you actually want to make clauses to check whether the current_date is > vis_date

I think it would be worth your while to learn a little about sqlalchemy and how we use that to make database queries.

Also, I would be much easier for me to see what you are doing if you just past the code using the triple backticks (check out github flavored markdown) You can also make a draft pull request so that I can see your actual code.

@jjjonesj
Copy link

jjjonesj commented Jul 3, 2024

Ah, sorry about the formatting. I'll fix it in the future, these were just plans that I did on stickynotes. However, could you look at the changes I made in assignSlice.js and assignment.jsx with a draft pull request? That's what I said did successfully yesterday but, I want to make sure it is up to your standards. I'll comment them properly if you approve of them afterward of course.

jjjonesj added a commit to kwizeras/rs that referenced this issue Jul 16, 2024
@moisedk
Copy link

moisedk commented Oct 25, 2024

Hello Dr. Brad @bnmnetp
I have been working on this issue and chose the option 1 you described above:

remove the is_visible column from the model. This would force any existing systems to run an update that would set the visible_on time to utcnow if is_visible is True. If is_visible is False then set visible_on to 10 years from now.
I made these changes:

  • In models.py: I removed the visible column and add a visible_on column for the visibility date with visible_on = Column(DateTime, default=datetime.utcnow)
  • In crud.py: I changed the visibility filter clause with vclause = Assignment.visible_on <= datetime.datetime.now() as this is used in the chooseAssignment endpoint inside student.py
  • In student.py, I updated the doAssignment endpoint to use only the visible_on column like this:
    assignment.visible_on is None or assignment.visible_on > datetime.datetime.utcnow()
    I then ran alembic revision --autogenerate -m "Remove visible and update visible_on column then upgrade head

However, when I restart the servers, I get an error saying that Assinment.visible does not exist.
My understanding is that this error is because I deleted the column, but there are other functions still referencing it.

Should I update every function that uses Assignement.visible to Assignement.visible_on?
I would also like to know if am doing anything wrong in the steps that I followed. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants