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

💻 Show the reason why you can see adventures #6134

Merged
merged 7 commits into from
Jan 27, 2025
Merged

💻 Show the reason why you can see adventures #6134

merged 7 commits into from
Jan 27, 2025

Conversation

rix0rrr
Copy link
Collaborator

@rix0rrr rix0rrr commented Jan 25, 2025

You can see adventures of teachers that you share classes with: both if you are the second teacher, and if they are the second teacher.

This is surprising at first, so this adds an icon that indicates why you can see a given adventure.

image

How to test

Log in as a user that shares a class with another teacher. Observe that you can see their adventures.

Closes #6133.

@rix0rrr rix0rrr requested a review from AnneliesVlaar January 25, 2025 14:13
@AnneliesVlaar
Copy link
Collaborator

The icon really helps!

I would expect to only see the custom adventures that are used in the shared class, would that be possible?

When clicking on a adventure from another teacher, you do go to the customization page, but it's not possible to edit the adventure (which is the expected behavior I think).

Can we remove the link behind the name of the adventure that goes to the customization page, and disable the remove button. Then the only thing one can do is preview the adventure?

@rix0rrr
Copy link
Collaborator Author

rix0rrr commented Jan 27, 2025

I would expect to only see the custom adventures that are used in the shared class, would that be possible?

Everything is possible 😆, it's just a matter of time. On first glance it seems like it shouldn't take too long, but I don't have the data model super clear in my head so it might be that there are dragons.

Can we remove the link behind the name of the adventure that goes to the customization page, and disable the remove button. Then the only thing one can do is preview the adventure?

I haven't checked, but I think the current explicitly implemented behavior is that if you share a class with someone, you can do everything to their adventures, including editing and deleting. (The reason I'm saying this is because I've seen calls to get_second_teacher_adventures() in many different places in the for_teachers file, including in the "deleting" code path).

While I agree with you that it seems unexpected, it seems to have been "expected behavior" by someone, because it was explicitly implemented that way. It might make sense as well: if you share a class with someone, you trust them to teach with you, and they might want to make a small improvement to some adventure. It would be annoying if they then had to wait for you to make the change.

These are good questions, that we might need to ponder a bit. Also, I don't think the current PR makes anything worse, you would just like to see more improvements. I propose we do the following:

  • If you think this PR makes the situation better, merge it as-is
  • Create 2 new issues for the 2 changes you would like to see (showing only adventures that are used in a class, second teachers only allowed to preview an adventure and nothing else)
  • Have a discussion on the additional issues and see if this is what we really want, and what the reason is for the current behavior being what it is
    • This could be a Discord discussion, and it could be informed by going back into the pull request history of the project, finding the PR that in introduced the current behavior, and seeing who made it (ask them) and seeing if there's any discussion on it

@rix0rrr
Copy link
Collaborator Author

rix0rrr commented Jan 27, 2025

I'll teach you a trick as well!

If you know what source file to look at (I happen to know that most of the teacher code that deals with adventures is in for_teachers.py), you can go to that file in GitHub:

2025-01-27 at 10 23

Then click "Blame" at the top:

2025-01-27 at 10 23

And then you can see for every line of code, who last changed it and in what PR.

I will then just Ctrl-F search in the file to look for things to do with "second adventures" (Ctrl-F for "second" or something to that effect), and then I come across this:

2025-01-27 at 10 24

And I can click through to the PR that introduced this code:

#4644

Written by Hasan. He's unfortunately not around anymore to ask, but he left some breadcrumbs:

2025-01-27 at 10 28

So I was wrong:

  • It's not possible to edit/delete an adventure you don't own.
  • But I think all adventures are visible so that a second teacher has the ability to add new adventures to the class that haven't been added to the class yet.

I would still argue that there are 2 new changes (and therefore GitHub issues) to make:

  • What adventures to list (Do we want to remove the ability for a 2nd teacher to add YOUR new adventures to both of your shared classes? Currently, I think the behavior might be that they can also add YOUR adventure to THEIR class... but is that even a bad thing? Presumably it's your colleague?)
  • Not showing the edit/delete buttons (that presumably send you to an error page anyway....?)

@boryanagoncharenko
Copy link
Collaborator

This PR is ready to go. @AnneliesVlaar if you agree with it, please feel free to approve it yourself.

Copy link
Contributor

mergify bot commented Jan 27, 2025

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).

Copy link
Contributor

mergify bot commented Jan 27, 2025

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 mergify bot merged commit 8d1af6d into main Jan 27, 2025
11 checks passed
@mergify mergify bot deleted the show-2nd-advs branch January 27, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🪲 Annelies can see Felienne's public adventures
3 participants