Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Allow teachers to share libraries with their classmates - #35738

#35738
Merged
jmkulwik merged 2 commits into
stagingcode-dot-org/code-dot-org:stagingfrom
fix-teacher-share-librariescode-dot-org/code-dot-org:fix-teacher-share-librariesCopy head branch name to clipboard
Jul 9, 2020
Merged

Allow teachers to share libraries with their classmates#35738
jmkulwik merged 2 commits into
stagingcode-dot-org/code-dot-org:stagingfrom
fix-teacher-share-librariescode-dot-org/code-dot-org:fix-teacher-share-librariesCopy head branch name to clipboard

Conversation

@jmkulwik

@jmkulwik jmkulwik commented Jul 9, 2020

Copy link
Copy Markdown
Contributor

We discovered that when we disabled auto-sharing of libraries from teachers to students, we also unintentionally disabled auto-sharing of libraries from teachers to classmates.
slack thread

This has updated our rules so teachers can auto-share libraries with classmates (For example: other teachers at a training)

Links

spec

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@jmkulwik
jmkulwik requested a review from a team July 9, 2020 19:01
project_owner = section_users.find {|user| user.id == student_storage_ids[project[:storage_id]]}
project_data = get_library_row_data(project, channel_id, section.name, project_owner)
if project_data && (project_owner.user_type == 'student' || project_data[:sharedWith].include?(section.id))
if project_data && (project_owner.id != section.user_id || project_data[:sharedWith].include?(section.id))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this change will still exclude the teacher's libraries (since section.user_id is the teacher's ID). i thought we wanted to include the teacher's libraries in this list?

related but separate: lines 129 and 132 (where we assign student_storage_ids and student_storage_id_list ) should probably be updated since section_users includes students and the teacher

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We want to include all classmates' libraries in the list, but we don't want to include the section owner's libraries in the list.

Details here:
https://docs.google.com/document/d/1zZ6WU5oBfh5Ra5TkdUpYRFsoMpIu9MRCWO42oBMyhrM/edit#

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ahh, my bad, i had to re-read the slack thread

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we cover this case with a unit test to prevent a regression?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

@maddiedierker maddiedierker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! thank you, jessie 😄

@jmkulwik
jmkulwik merged commit 201165c into staging Jul 9, 2020
@jmkulwik
jmkulwik deleted the fix-teacher-share-libraries branch July 9, 2020 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.