Skip to content

Grant admin rights to members of the course team - #470

Merged
singingwolfboy merged 36 commits into
masterfrom
db/course-team-admin-grants
Jul 30, 2013
Merged

Grant admin rights to members of the course team#470
singingwolfboy merged 36 commits into
masterfrom
db/course-team-admin-grants

Conversation

@singingwolfboy

Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is this testing? I'm not clear what self.index_url is supposed to signify.

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.

It's the URL for the manage_users page. Is there a clearer name I can use?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

manage_users_url?

I was just confused because I thought you were trying to test the index page (ie, index.html, which is what our dashboard page is). I saw that you had it set to manage_users, but then I couldn't figure out what you were trying to test. You are checking that the user is not on the course initially?

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.

The "index/detail pages" pattern is very common in web development. An index page provides a list view of many resources, while a detail page goes into detail about one specific resource. I would have changed the name of the view, but that would have required changing it all over the site, and I wasn't sure that was a good idea.

The testing ambiguity is addressed in 0dbb458.

@cahrens cahrens closed this Jul 23, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't lose Peter's bug fix. Having a leading space on an e-mail address doesn't seem to work on your branch.

Although I also can't add users in general on your branch (I get an error).

@cahrens

cahrens commented Jul 23, 2013

Copy link
Copy Markdown

A few things--

  1. If you have a user marked "is_staff", then on the manage users page the user is showing up as having admin rights for the course (and you can't revoke those rights). The intersection of is_staff and instructor group is problematic.
  2. We don't want to allow removing admin rights from the last admin person.

singingwolfboy and others added 23 commits July 29, 2013 13:11
Match other views better, saner URLs, more RESTful style, extensible for other roles
It was causing unit tests to fail, and it's a needless bit of abstraction that
never should have existed in the first place.
A user with `is_staff=True` is treated as being in all groups. This is problematic
when we care about the user's staff/instructor role for a course: you can't remove
the instructor role. This commit changes the `is_user_in_course_group_role` function
to allow the caller to specify that it should not check the `is_staff` attribute
on the user.
@peter-fogg

Copy link
Copy Markdown
Contributor

If the filler text in manage_users.html is OK, 👍.

Comment thread cms/templates/manage_users.html Outdated

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.

What should this be? It probably shouldn't be "Textbook information".

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.

D'oh! That's an artifact from copying and pasting a design pattern from the Textbook UI. I'll fix that up quickly.

@cahrens

cahrens commented Jul 30, 2013

Copy link
Copy Markdown

@singingwolfboy Don't forget to add a manual BDD spec for our manual test plans (see https://edx-wiki.atlassian.net/wiki/display/STU/Studio+BDD+Specs+index?src=search). In particular, highlight anything that is not covered by an automated test. If everything is covered, you can use your Lettuce BDD spec.

singingwolfboy added a commit that referenced this pull request Jul 30, 2013
Grant admin rights to members of the course team
@singingwolfboy
singingwolfboy merged commit 7d7513b into master Jul 30, 2013
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
smarnach referenced this pull request in open-craft/openedx-platform Aug 4, 2015
API call to display 'is_staff' attribute
hachiyanagi-ks added a commit to nttks/edx-platform that referenced this pull request Nov 16, 2015
hachiyanagi-ks added a commit to nttks/edx-platform that referenced this pull request Nov 16, 2015
…s-header-logo

Enable to configure the header logo openedx#470
diegomillan pushed a commit to eduNEXT/edx-platform that referenced this pull request Sep 14, 2016
…nt_response_report_for_content_libs

Add support for content lib problems in student response report.
xavierchan pushed a commit to xavierchan/edx-platform-1 that referenced this pull request Aug 19, 2019
fix(account-setting page): add a link to manual page
yoann-mroz pushed a commit to weuplearning/edx-platform that referenced this pull request Nov 30, 2020
'make dev.provision' was trying to provision
the 'o' service, due strangeness related to the fact
that Makefiles were designed for compiling C
programs (which have .o intermediate files).

Fix the issue by adding all rules to PHONIES,
which tells Make that none of the targets are
real files.

Co-authored-by: Tim McCormack <tmccormack@edx.org>
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
Danyal-Faheem pushed a commit to Danyal-Faheem/edx-platform that referenced this pull request Jul 15, 2025
@farhan farhan added the depr Proposal for deprecation & removal per OEP-21 label Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

depr Proposal for deprecation & removal per OEP-21

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants