Details

      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)

        Gliffy Diagrams

          Activity

          Hide
          scriby 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
          scriby 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
          salvetore Michael de Raadt added a comment -

          Thanks for working on this Chris and Eloy.

          Show
          salvetore Michael de Raadt added a comment - Thanks for working on this Chris and Eloy.
          Hide
          stronk7 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
          stronk7 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
          stronk7 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
          stronk7 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
          scriby 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
          scriby 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
          stronk7 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
          stronk7 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
          scriby 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
          scriby 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
          stronk7 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
          stronk7 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          stronk7 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
          stronk7 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
          stronk7 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
          stronk7 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:
                Fix Release Date:
                5/Dec/11