Details

    • Rank:
      32704

      Description

      1. When configuring a External Tool activity, the privacy settings from site / course level tool providers should restrict privacy controls in the External Tool configuration.

      2. Remove the privacy setting for roster access (not implemented in LTI 1.1)

        Activity

        Hide
        Chris Scribner added a comment - - edited

        Code is in branch: https://github.com/scriby/moodle/commits/MDL-30344

        When merging this change into core, don't include the b06540b4058a1a37a1a364ce0a71585e27b2d664 revision. It's included in another issue, but was required for the changes to apply cleanly against this branch.

        Show
        Chris Scribner added a comment - - edited Code is in branch: https://github.com/scriby/moodle/commits/MDL-30344 When merging this change into core, don't include the b06540b4058a1a37a1a364ce0a71585e27b2d664 revision. It's included in another issue, but was required for the changes to apply cleanly against this branch.
        Hide
        Michael de Raadt added a comment -

        Thanks for working on this Chris and Eloy.

        Show
        Michael de Raadt added a comment - Thanks for working on this Chris and Eloy.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Chris,

        just one side comment, you don't need to make me assignee of the issue, the better is to keep yourself in that role.

        Whenever you request any peer-review, just add the peer-reviewiever (me) and I'll be notified.

        I've just finished running one integration cycle some mins ago. Tomorrow morning (EU) I'll be devoted to all these issues, many thanks for the hard work!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Chris, just one side comment, you don't need to make me assignee of the issue, the better is to keep yourself in that role. Whenever you request any peer-review, just add the peer-reviewiever (me) and I'll be notified. I've just finished running one integration cycle some mins ago. Tomorrow morning (EU) I'll be devoted to all these issues, many thanks for the hard work! Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        One more detail... in the commit message, try to put always the correct MDL-xxxx issue the commit is related to. I've seen that you are using MDL-20534 in the commits belonging to this (MDL-30344).

        Not a cause for rejection @ this stage (no change required this time), but in the long term, better each commit with its issue, it helps to identify the issue far better.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - One more detail... in the commit message, try to put always the correct MDL-xxxx issue the commit is related to. I've seen that you are using MDL-20534 in the commits belonging to this ( MDL-30344 ). Not a cause for rejection @ this stage (no change required this time), but in the long term, better each commit with its issue, it helps to identify the issue far better. Ciao
        Hide
        Chris Scribner added a comment -

        Sorry about the wrong issue numbers. I did these commits before moving to the tracker process. I'll get them right from here on out.

        Show
        Chris Scribner added a comment - Sorry about the wrong issue numbers. I did these commits before moving to the tracker process. I'll get them right from here on out.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Ops, I'm getting conflicts here. Could you please:

        1) Take out b06540b4 (the big lang changes - MDL-30339) from this branch. It has been already integrated and one extra commit was added after that, leading to conflict here.

        2) Point me to the issue where b06540b4058a1a37a1a364ce0a71585e27b2d664 belongs to, in order to get it integrated before this.

        TIA!

        Show
        Eloy Lafuente (stronk7) added a comment - Ops, I'm getting conflicts here. Could you please: 1) Take out b06540b4 (the big lang changes - MDL-30339 ) from this branch. It has been already integrated and one extra commit was added after that, leading to conflict here. 2) Point me to the issue where b06540b4058a1a37a1a364ce0a71585e27b2d664 belongs to, in order to get it integrated before this. TIA!
        Hide
        Chris Scribner added a comment -

        Feel free to cherry-pick the changes in, I don't need to maintain mergability with my branch. I've changed my approach to avoid this problem in the future.

        Show
        Chris Scribner added a comment - Feel free to cherry-pick the changes in, I don't need to maintain mergability with my branch. I've changed my approach to avoid this problem in the future.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I've reviewed and tested this a bit and seems to be working great. My unique 3 concerns are:

        • the same behavior we use for privacy settings right now (ajax-request & enable/disable...) should be also used for other pieces of information like "consumer key", "shared secret" and, perhaps also "custom parameters". I can imagine one admin setting those at site level and wanting teachers to don't know about them.
        • when the site/course tool is shown in the activitie's drop down and picked... should the teacher still be able to edit the url? Or, if picked the template URL should be assumed? I mean, we are mixing two ways to configure the activity tool, by automatic matching (this requires typing the url in the activity, for sure) and by picking in the dropdown menu one existing template (if this template includes one url... should it be used and the activity url disabled?).
        • the message shown on the right of the URLs in the activity tool shows right now:
          "Using tool configuration" (for automatically matching and picked ones) and "Using custom tool configuration" (for non-matching ones). Just guessing if, to be a bit more "moodle-like" we could name them: "Using site tool", "Using course tool" and "Using activity tool". That provides extra info about where is the matching tool (site/course) and is more moodle-slang.

        Feel free to create new issues about this. In the meantime I'm going to integrate this one as is (with one extra commit cleaning incorrect whitespace and comments).

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I've reviewed and tested this a bit and seems to be working great. My unique 3 concerns are: the same behavior we use for privacy settings right now (ajax-request & enable/disable...) should be also used for other pieces of information like "consumer key", "shared secret" and, perhaps also "custom parameters". I can imagine one admin setting those at site level and wanting teachers to don't know about them. when the site/course tool is shown in the activitie's drop down and picked... should the teacher still be able to edit the url? Or, if picked the template URL should be assumed? I mean, we are mixing two ways to configure the activity tool, by automatic matching (this requires typing the url in the activity, for sure) and by picking in the dropdown menu one existing template (if this template includes one url... should it be used and the activity url disabled?). the message shown on the right of the URLs in the activity tool shows right now: "Using tool configuration" (for automatically matching and picked ones) and "Using custom tool configuration" (for non-matching ones). Just guessing if, to be a bit more "moodle-like" we could name them: "Using site tool", "Using course tool" and "Using activity tool". That provides extra info about where is the matching tool (site/course) and is more moodle-slang. Feel free to create new issues about this. In the meantime I'm going to integrate this one as is (with one extra commit cleaning incorrect whitespace and comments). TIA and ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Tested automatisms between site/course tools (templates) and activity values, seems way more clear and natural now, yay!

        But the observations shared in my previous comment, plz don't forget them. Adding some people here... let's discuss and agree if we need to go one step further...

        Show
        Eloy Lafuente (stronk7) added a comment - Tested automatisms between site/course tools (templates) and activity values, seems way more clear and natural now, yay! But the observations shared in my previous comment, plz don't forget them. Adding some people here... let's discuss and agree if we need to go one step further...
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Closing as fixed, many thanks for your effort!

        Note that the changes related to master (2.2beta) have been already sent upstream. But the stable ones will be part of next weeklies (Wed/Thu) as usual.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your effort! Note that the changes related to master (2.2beta) have been already sent upstream. But the stable ones will be part of next weeklies (Wed/Thu) as usual. Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: