🪲 Show unsubmit button to teachers only for programs of their current students

[boryanagoncharenko]

This PR makes 3 changes:

1. Until now the checkbox for approving student adventures was only shown to the primary teacher. Now the checkbox is also shown to secondary teachers.
2. The unsubmit button is only displayed if the logged-in user is a current teacher (primary or secondary) of the student who submitted the program.
3. The statement that the program is submitted and cannot be altered together with the button to unsubmit have been moved, so that they do not take the upper central part of the screen.

Fixes #5990

How to test

Test user permissions

• Do you agree that a secondary teacher of the class should be able to unsubmit and/or approve submitted programs?
• A primary teacher can unsubmit the program of their student: log in as a student (student1) enrolled in a class and submit a program. Then log in as the primary teacher (teacher1) and check that you can unsubmit that program, check that you cannot unsubmit a program of the same student which has not been submitted.
• A secondary teacher can unsubmit the program of their student: log in as a student (student1) enrolled in a class and submit a program. Then log in as the secondary teacher (e.g. teacher4) and check that you can unsubmit that program.
• A teacher cannot unsubmit the program of a student who is not enrolled in their class: log in as a student (student1) enrolled in a class and submit a program. Then log in a teacher who is not a teacher of the student's class (teacher2) and check that you cannot unsubmit that program. Note that you should be able to see if a program is submitted and when.
• A student cannot unsubmit the program of another student: log in as another student (student2) and check that you cannot unsubmit that program. Again, you should be able to see if a program is submitted and when

Check whether you agree with the layout. Please note that this page uses too many headings, different styles and overall does not look stylistically coherent. I intentionally made the minimal changes in the layout without changing styling. It would be great to improve its look and feel in the future. I can also revert all layout changes, as I am not sure whether the new layout is better than the old one.

This is how a submitted program looks if you are a teacher of the student:

And this is how if looks when you are another student or a teacher who is not in the student's class:

Note that when you unsubmit a program, the UI should be updated, so that you cannot unsubmit an already unsubmitted program.

[rix0rrr]

There is a concept here that I'd like to see expressed in code rather than procedurally. This:

def students_teachers(student):
  ...

def is_students_teacher(student, teacher):
  return teacher in students_teachers(student)

If we make it a function, we can reuse the same logic in other places. I'm sure the exact same logic is going to be used somewhere where we determine if a user can view a saved program, no?

def can_view_program(user, program):
  return program['owner'] == user or is_students_teacher(program['owner'], user)

Right now, what do we do for determining whether a program is viewable by someone?

In all cases of code everywhere, I would encourage thinking about the fundamental operations that are being done and making functions out of them, rather than pasting in more lines of just-so data structure manipulation.

[rix0rrr]

Thanks for making the change! It occurred to me that this check, or one like it, must already have happened when we determine whether or not the current user is allowed to look at this program in the first place.

And indeed, it happens here:

hedy/app.py

Lines 2939 to 2960 in ac62a54

	def current_user_allowed_to_see_program(program):
	"""Check if the current user is allowed to see the given program.
	Verify that the program is either public, the current user is the
	creator, teacher or the user is admin.
	"""
	user = current_user()
	# These are all easy
	if program.get('public'):
	return True
	if user['username'] == program['username']:
	return True
	if is_admin(user):
	return True
	if is_teacher(user) and program['username'] in DATABASE.get_teacher_students(user['username']):
	return True
	return False

This means we're now calling get_students_teachers/get_teacher_students twice, which is unnecessary database traffic (the fact that they're different also makes it possible for them to disagree, which might be confusing at some point).

To save on this, can we instead return some more information from current_user_allowed_to_see_program: not only yes/no, but also what kinds of things the user might do to to the program?

Can we return something like:

{ 
  "can_view": True|False,
  "can_change": True|False,
}

Instead, and use that? Or maybe a ternary or enum, to express the states False | 'can_view' | 'can_change' ?

This might require changing the order of the conditions in current_user_allowed_to_see_program around.

Because in the order we do it now, we early exit if the program is public. The only thing we would normally be able to conclude from that early exit is can_view... but we wouldn't want that to block a teacher's ability to unsubmit the program, so we would need to test the teacher/admin conditions before testing the public condition.

[boryanagoncharenko]

I made the change you requested but this is not a good state to leave the codebase.

If I understand correctly, submitting/unsubmitting/checking(which is kind of like approving) a program is not the same as editing it. The program page has an Edit button but no matter what I do, I can never actually edit the program but maybe this is me not understanding how it should work. I used a named tuple with an unsubmit permission, just to ensure it will not be confused with the write permission.

Additionally, the only way to see program page is if the program is public. Of course, you can hit the URL directly, but if you are only using the interface of Hedy, you will not end up on this page unless it is public. So, we do not call the get_student_teachers-like logic twice. At the same time, this page is currently the only one requiring can_unsubmit access. All other uses of the current_user_allowed_to_see_program function check only for read access. Because of the reordering of rules within the function, it seems that now we actually end up with more database calls than before.

Please let me know if my assumptions about the functionality are correct and, also, how you would like me to proceed.

[rix0rrr]

If I understand correctly, submitting/unsubmitting/checking(which is kind of like approving) a program is not the same as editing it.

Fair enough. Maybe there are 4 levels of permissions: none | can_view | can_unsubmit | can_edit. I meant this more as an example to convey that they are not independent: can_edit implies can_unsubmit and can_unsubmit implies can_view.

On the other hand, can_edit does not imply can_checkoff, so there might be some additional states we would need to be able to represent. Maybe the datastructure would be:

@dataclass
class ProgramPermissions:
  can_unsubmit: bool
  can_checkoff: bool
  can_edit: bool


can_checkoff = is_teacher
can_edit = is_author
can_unsubmit = is_author or is_teacher or is_admin

The presence of a value indicates can_view (because the other permissions don't make sense without viewing). A value of None indicates that a user can't view it. That also means we don't need to change most of the call sites to do the tuple unpacking, because they only test for truthiness of the return value.

Additionally, the only way to see program page is if the program is public.

If I'm understanding it correctly:

• Authors of a program can view their own programs using a link on the my_programs page.
• Teachers can look at the my_programs page of all students they teach and find this page that way.
• Teachers using the teacher interface can use the grading queue to hit this page as well

So to my understanding, a program being public isn't the only way to end up here?

So, we do not call the get_student_teachers-like logic twice.

But didn't it use to look like this:

def view_program():
    # This wraps a call to check whether the current user is a teacher of the author
    if not result or not current_user_allowed_to_see_program(result):  
        return utils.error_page(error=404, ui_message=gettext('no_such_program'))
  
    # ...

    # Second call to c heck whether the current user is a teacher of the author
    arguments_dict['is_students_teacher'] = is_students_teacher(student=result['username'], teacher=user['username'])
 
    # ...

Am I misreading something?

Because of the reordering of rules within the function, it seems that now we actually end up with more database calls than before.

Fair enough, you are saying that for public programs, we are now doing a database lookup to determine whether we can view a program, even if we didn't need to know whether we can unsubmit it.

But I think we can deal with that another way: right now we are doing the database query to see if the current user is the teacher of someone unconditionally, even if the current user is not a teacher (and the database lookup would presumably always return False anyway).

We could simplify it by not doing the lookup unless user['is_teacher'] (or user['teacher'], whichever one it is). Saves us doing the lookup for 99% of the cases. If we have that, I'm comfortable enough having that additional bit of information even though we don't strictly need it.

[boryanagoncharenko]

If I'm understanding it correctly:

Authors of a program can view their own programs using a link on the my_programs page.
Teachers can look at the my_programs page of all students they teach and find this page that way.
Teachers using the teacher interface can use the grading queue to hit this page as well

Apologies for the extra detailed discussion, but I need to get this straight because the teachers panel the current pain-point.

Based on your comment, I figured that if I am a teacher I can see a non-public, non-submitted program of a student via For Teachers > Class and then use the eye icon in the Student's Adventures table.

However, if I am a student, I click on My programs and I am on the My recent programs page, then I cannot get to the hedy/<program-id>/view page unless the program is public. Right under every program there is an Open button which opens the program source in a hedy/1/<program-id> page. If the program is public, then in the More options menu of the program appears a Copy link to share item which is a reference to the hedy/1/<program-id>. Exactly the same interface is used when I am a teacher and I look at the my_programs page of students. Am I missing something on this page?

But didn't it use to look like this:

Am I misreading something?

You are not misreading. My comment was based on the assumption that the view-program-page is for public programs. Overall, I am confused about the the possible states of programs, the grading mechanism and overall about the teachers' panel. Sorry about that.

Other than that, I made the changes you requested.

[rix0rrr]

Am I missing something on this page?

It's very likely that I am the one missing something here. To confirm, because I trust you have a better view on this than I do:

• The view_program page is used for public programs and when a teacher uses the eye icon to look at a student's program.
• All other instances of users looking at programs (either their own or other's) go to the "normal" code page.

Does that seem (weird but) accurate to you?

---

While the different views that are being used for these code paths seem a bit odd to me, I'd like to have the "what code makes decisions" and "how we render it" on different axes. And especially the code that makes decisions should be centralized as much as possible, so it's in one obvious place if we ever need to change rules. So I still feel confident that the changes we're making are for the better.

What do you think?

[boryanagoncharenko]

Yes, of course, having common access rules is a natural choice. Thanks for pointing out there are other paths to the program page.

[rix0rrr]

Thank you!

[mergify]

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

[mergify]

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).