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

Share course with specific users #2204

Merged
merged 34 commits into from
Feb 26, 2019
Merged

Share course with specific users #2204

merged 34 commits into from
Feb 26, 2019

Conversation

taylortom
Copy link
Member

@taylortom taylortom commented Jan 28, 2019

Fixes #2123 (framework issue #2345).

Also removed the unused shared project view and associated files.

@taylortom taylortom added this to the 0.7.0 Loose ends milestone Jan 28, 2019
"help": "Controls whether or not your colleagues will be able to see this course from the 'Shared courses' option"
},
"_shareWithUsers": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Adds extra sharing field

Copy link
Contributor

Choose a reason for hiding this comment

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

TomG's PR is going to mean that these schemas get pulled in from the framework (#2185) so I think that putting this here will mean that it is overwritten

Copy link
Member Author

Choose a reason for hiding this comment

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

Have opened a framework PR here: adaptlearning/adapt_framework#2346

@taylortom taylortom changed the title Feature/sharing Share course with specific users Jan 28, 2019
@taylortom taylortom changed the base branch from develop to release/0.7.0 January 28, 2019 13:22
"help": "Controls whether or not your colleagues will be able to see this course from the 'Shared courses' option"
},
"_shareWithUsers": {
Copy link
Contributor

Choose a reason for hiding this comment

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

TomG's PR is going to mean that these schemas get pulled in from the framework (#2185) so I think that putting this here will mean that it is overwritten

lib/helpers.js Outdated Show resolved Hide resolved
@lc-thomasberger
Copy link
Member

lc-thomasberger commented Feb 14, 2019

When I share a course with all users, other users can't see the course.
Only if it is shared with a certain user the course is displayed in "shared courses".

@lc-thomasberger
Copy link
Member

A course that is shared with all users can't be deleted by the user that created it.

@taylortom
Copy link
Member Author

This is working with #1926, so will get that one merged first before looking again.

# Conflicts:
#	frontend/src/modules/projects/index.js
#	frontend/src/modules/projects/views/projectsView.js
#	frontend/src/modules/scaffold/index.js
#	lib/helpers.js
#	plugins/content/course/index.js
@taylortom taylortom added the S: in-progress Issues currently being worked on - leave these be! label Feb 20, 2019
@taylortom taylortom removed the S: in-progress Issues currently being worked on - leave these be! label Feb 20, 2019
@canstudios-nicolaw
Copy link
Contributor

canstudios-nicolaw commented Feb 20, 2019

@taylortom Would you expect anything to be required to test this other than npm install, grunt build and a restart?

I am getting a lot of errors in the console when I try and view dashboards or courses

screenshot from 2019-02-20 13 57 26

I can see _shareWithUsers in my course model.schema, I'm testing on an install that existed before TomG's schema release so I think it is using the schemas in the authoring tool.

I will try and investigate this now but just wanted to check I'm not missing something


Update: I hadn't realised we need to test with a version of the framework that contains the schema change (adaptlearning/adapt_framework#2351 or adaptlearning/adapt_framework#2346)

@canstudios-nicolaw
Copy link
Contributor

canstudios-nicolaw commented Feb 20, 2019

  1. If I log in and share the course with a user, then that user does appear to be able to edit, delete, download and export the course but they cannot preview it - this gives a 401 permission denied

  2. If I share the course with User A, then User A can share the course with everyone. This is a new scenario that we won't have come across before. Should the course owner be the only person who can share a course with everyone?

  3. I think it's a little strange that if I share a course with somebody I don't really have any way to see that from the dashboard. I think I would have expected to see it on the shared courses dashboard because it has been shared with somebody. The sharing settings are right next to each other in the project settings so I would hope that upon opening the course from there it would be easy to determine the type of sharing. I'm open to other opinions on this

@taylortom
Copy link
Member Author

taylortom commented Feb 21, 2019

Good questions,

  1. Will have a look into this one.
  2. This is as designed, the thinking being if you have permission to edit (and delete) a course, then why not permission to change the sharing settings. If we agree on a different strategy, happy to change it.
  3. I think this is more a failing of the dashboard in general - I've never liked the separate pages for mine/shared. Think a better approach would be to merge the two, and use filters/views to alter what's displayed. I also think we need a set of icons to show more info about each course (i.e. mine, shared, ... possibly other stuff)

@canstudios-nicolaw
Copy link
Contributor

Ok thanks @taylortom

I'm fine with 2 & 3 for now then 👍

Copy link
Contributor

@canstudios-nicolaw canstudios-nicolaw left a comment

Choose a reason for hiding this comment

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

If I tick the 'share with all users' box and don't share the course with any specific user then:

  • I cannot see the course in the shared courses dashboard
  • Super admins in the system can see the course on the shared courses dashboard
  • Course creators in the system cannot see the course on the shared courses dashboard

@lc-thomasberger
Copy link
Member

I can replicate @canstudios-nicolaw issue above.

# Conflicts:
#	frontend/src/modules/projects/index.js
#	frontend/src/modules/projects/views/projectsView.js
#	lib/helpers.js
#	plugins/content/course/index.js
@canstudios-nicolaw
Copy link
Contributor

@taylortom I'm still having one of the issues above, if I share a course via the 'share with all' checkbox then I don't see it on my shared courses dashboard. This issue occurs whether I have also shared with some individuals or not. The rest is working well now though 👍

@lc-thomasberger
Copy link
Member

I can not replicate. @canstudios-nicolaw can you please check your tenants?

@taylortom
Copy link
Member Author

taylortom commented Feb 26, 2019

@lc-thomasberger @canstudios-nicolaw I think this is related to courses which you own? I changed the API query slightly to omit any shared courses not owned by you (as they're already shown in 'My courses'). Will revert this in light of the upcoming #2238

@taylortom taylortom merged commit b074e78 into adaptlearning:release/0.7.0 Feb 26, 2019
@taylortom taylortom deleted the feature/sharing branch February 26, 2019 14:16
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

Successfully merging this pull request may close these issues.

4 participants