Moodle
  1. Moodle
  2. MDL-36704

Wrap activity condition rules in some tag

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Duplicate
    • Affects Version/s: 2.3.3
    • Fix Version/s: None
    • Component/s: Activity completion
    • Labels:
    • Testing Instructions:
      Hide

      1. Set a date condition on an activity so that it is not available to students (it should be set to display but greyed-out).
      2. Log in as a student.
      3. Using tools in your browser, examine the HTML code for the text that warns about the activity not being available yet.

      • It should be contained in a span with the classes 'conditionalrule requires_date'
        4. Run unit tests for conditionlib (or as part of all the unit tests)
      • Unit tests for conditionlib should pass.
      Show
      1. Set a date condition on an activity so that it is not available to students (it should be set to display but greyed-out). 2. Log in as a student. 3. Using tools in your browser, examine the HTML code for the text that warns about the activity not being available yet. It should be contained in a span with the classes 'conditionalrule requires_date' 4. Run unit tests for conditionlib (or as part of all the unit tests) Unit tests for conditionlib should pass.
    • Affected Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-36704-master
    • Rank:
      46207

      Description

      If an activity has a lot of rules for conditional availability, you tend to get a wall of text that's pretty impossible to read. Here's a simple patch that just wraps each one in a <span> tag with some classes to allow a theme to display them a little cleaner.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Hi Jay,

          This is a good idea. I agree with these changes and am happy to submit it for integration (I guess as this is an improvement it will only go into master i.e. 2.5? Do you want to push for it being in 2.4 as well like a bugfix?) - but first please could you adjust the following minor details and update your patch:

          1. Instead of writing HTML code in strings, please could you use html_writer to output the new spans. I know the existing code in this area all does HTML manually (and I am definitely not suggesting that you should change anything else), but I think it's better to use html_writer for new/modified code lines. This will probably make it slightly longer (you might have to wrap some lines) but it shouldn't be too horrible.

          2. Could you change the name of the date classes to be consistent with the naming of the fields, i.e. use 'from' and 'until' instead of nothing and 'after'?

          Also - I think there are existing unit tests in this area - could you check that unit tests still pass, and if necessary, update them to pass with the new spans. Even if they already pass, if there is a natural place to add it, it would be good to check for the classes being correct. (Probably just with a text match to see if the string "conditionalrule requires_completion" is present, that sort of thing.)

          Show
          Sam Marshall added a comment - Hi Jay, This is a good idea. I agree with these changes and am happy to submit it for integration (I guess as this is an improvement it will only go into master i.e. 2.5? Do you want to push for it being in 2.4 as well like a bugfix?) - but first please could you adjust the following minor details and update your patch: 1. Instead of writing HTML code in strings, please could you use html_writer to output the new spans. I know the existing code in this area all does HTML manually (and I am definitely not suggesting that you should change anything else), but I think it's better to use html_writer for new/modified code lines. This will probably make it slightly longer (you might have to wrap some lines) but it shouldn't be too horrible. 2. Could you change the name of the date classes to be consistent with the naming of the fields, i.e. use 'from' and 'until' instead of nothing and 'after'? Also - I think there are existing unit tests in this area - could you check that unit tests still pass, and if necessary, update them to pass with the new spans. Even if they already pass, if there is a natural place to add it, it would be good to check for the classes being correct. (Probably just with a text match to see if the string "conditionalrule requires_completion" is present, that sort of thing.)
          Hide
          Jay Knight added a comment -

          I've updated my branch with proper (I think) use use of html_writer, and updated the tests accordingly (and they all pass).

          I'm not completely sure what you're talking about in #2. I am generally using the get_string() name as the classname, though sometimes just using the "base" where it is dynamic.

          2.5 will be okay. We've lived with it for a while, we'll deal a little longer

          Show
          Jay Knight added a comment - I've updated my branch with proper (I think) use use of html_writer, and updated the tests accordingly (and they all pass). I'm not completely sure what you're talking about in #2. I am generally using the get_string() name as the classname, though sometimes just using the "base" where it is dynamic. 2.5 will be okay. We've lived with it for a while, we'll deal a little longer
          Hide
          Sam Marshall added a comment -

          Thanks Jay. Sorry, I see what you mean with #2 - basically your change is completely consistent with the string names (which are not consistent with the database field names, but it's way too late to change that). I withdraw that comment.

          Also thanks for the git patch. Just ONE little thing left - please could you check there are spaces after commas. Mostly you have them but there are just a few lines like this one:

          html_writer::tag('span',get_stri

          where there is a missing space. (Note: If you use the local_codechecker tool https://github.com/moodlehq/moodle-local_codechecker it will automatically report on that kind of trivia. It will also find like a bazillion other things wrong in those existing files, so it'll be nearly impossible to read the report in this case as there is unfortunately no easy way to get it to run on just the changed lines...)

          Show
          Sam Marshall added a comment - Thanks Jay. Sorry, I see what you mean with #2 - basically your change is completely consistent with the string names (which are not consistent with the database field names, but it's way too late to change that). I withdraw that comment. Also thanks for the git patch. Just ONE little thing left - please could you check there are spaces after commas. Mostly you have them but there are just a few lines like this one: html_writer::tag('span',get_stri where there is a missing space. (Note: If you use the local_codechecker tool https://github.com/moodlehq/moodle-local_codechecker it will automatically report on that kind of trivia. It will also find like a bazillion other things wrong in those existing files, so it'll be nearly impossible to read the report in this case as there is unfortunately no easy way to get it to run on just the changed lines...)
          Hide
          Jay Knight added a comment -

          My code now passes the codechecker.

          Show
          Jay Knight added a comment - My code now passes the codechecker.
          Hide
          Sam Marshall added a comment -

          Thanks Jay. I'm just tidying up your branch for submission, so I've put it into my GitHub account. For future reference (so you can do it yourself in future if required), the things that I changed:

          1. There needs to be a branch based on master (at least) as well as branches for any stable versions you want the change in.
          2. The Git url for the repository (git:...) needs to go in the 'Pull from repository' box.
          3. The 'Pull ... branch' box takes the branch name, not a URL
          4. You had two commits for the change. Moodle allows this if necessary, but in that case you need to make sure both commit comments have the issue number. I didn't think there was any benefit keeping it separate, so I combined them.

          There's one other things I changed that isn't required: I use a particular way of naming branches but this is only my own convention, nobody else does it.

          Show
          Sam Marshall added a comment - Thanks Jay. I'm just tidying up your branch for submission, so I've put it into my GitHub account. For future reference (so you can do it yourself in future if required), the things that I changed: 1. There needs to be a branch based on master (at least) as well as branches for any stable versions you want the change in. 2. The Git url for the repository (git:...) needs to go in the 'Pull from repository' box. 3. The 'Pull ... branch' box takes the branch name, not a URL 4. You had two commits for the change. Moodle allows this if necessary, but in that case you need to make sure both commit comments have the issue number. I didn't think there was any benefit keeping it separate, so I combined them. There's one other things I changed that isn't required: I use a particular way of naming branches but this is only my own convention, nobody else does it.
          Hide
          Sam Marshall added a comment -

          Submitting for integration.

          Note: I guess probably nobody will look at this until 2.5 development starts since it is a low priority change. Although there is a branch for 2.3, I leave it up to the integrator to decide which branches it goes into, whether just master or others (the code in this area hasn't changed recently so it will cherry-pick fine).

          Show
          Sam Marshall added a comment - Submitting for integration. Note: I guess probably nobody will look at this until 2.5 development starts since it is a low priority change. Although there is a branch for 2.3, I leave it up to the integrator to decide which branches it goes into, whether just master or others (the code in this area hasn't changed recently so it will cherry-pick fine).
          Hide
          Dan Poltawski added a comment -

          I've integrated this to 2.4 and master only.

          Its boderline as to whether this should go into 2.4 as i'd say its an improvement rather than a bugfix, but we are being more liberal on the 2.4 branch whilst its new.

          Show
          Dan Poltawski added a comment - I've integrated this to 2.4 and master only. Its boderline as to whether this should go into 2.4 as i'd say its an improvement rather than a bugfix, but we are being more liberal on the 2.4 branch whilst its new.
          Hide
          Rajesh Taneja added a comment -

          Sorry Sam,
          Not sure if this is correct behaviour.
          – As student class is "conditionalrule availablefrom"
          – As Teacher class is "conditionalrule requires_date"

          failing this as test instructions says as student you should see "conditionalrule requires_date".

          FYI: Unit test pass.

          Show
          Rajesh Taneja added a comment - Sorry Sam, Not sure if this is correct behaviour. – As student class is "conditionalrule availablefrom" – As Teacher class is "conditionalrule requires_date" failing this as test instructions says as student you should see "conditionalrule requires_date". FYI: Unit test pass.
          Hide
          Sam Marshall added a comment -

          Argh. I looked at this and actually coded a fix for the minor problem, but please can this change be rejected and removed from integration - I have just noticed that it conflicts with another change that is also due for integration review and will address the same problem...! (The other one is a more complete change; it makes the items into a proper HTML list.)

          I don't know how I failed to notice that I submitted two issues to solve the same problem. My apologies to everyone involved for the wasted time here.

          The other change is MDL-36991. I suggest we resolve this issue as duplicate as that - unless anyone is dissatisfied with the result of that change. (Because that one doesn't actually allow for the different types of condition to be styled differently, it just means they'll appear one-per-line in a list; we could add in the classes to that change if required, combining both changes. But I think that probably isn't necessary.)

          Show
          Sam Marshall added a comment - Argh. I looked at this and actually coded a fix for the minor problem, but please can this change be rejected and removed from integration - I have just noticed that it conflicts with another change that is also due for integration review and will address the same problem...! (The other one is a more complete change; it makes the items into a proper HTML list.) I don't know how I failed to notice that I submitted two issues to solve the same problem. My apologies to everyone involved for the wasted time here. The other change is MDL-36991 . I suggest we resolve this issue as duplicate as that - unless anyone is dissatisfied with the result of that change. (Because that one doesn't actually allow for the different types of condition to be styled differently, it just means they'll appear one-per-line in a list; we could add in the classes to that change if required, combining both changes. But I think that probably isn't necessary.)
          Hide
          Jay Knight added a comment -

          I don't know how I failed to notice that I submitted two issues to solve the same problem.

          Sam, I submitted this one with an initial patch... but I like the other one better. I was going to style them as a list anyway, but wanted to keep the default/unstyled display the same (for better chance of it being accepted).

          I don't really need the different classes, but thought it wouldn't hurt to throw those in as well, you can add them to the other change if you want to

          Show
          Jay Knight added a comment - I don't know how I failed to notice that I submitted two issues to solve the same problem. Sam, I submitted this one with an initial patch... but I like the other one better. I was going to style them as a list anyway, but wanted to keep the default/unstyled display the same (for better chance of it being accepted). I don't really need the different classes, but thought it wouldn't hurt to throw those in as well, you can add them to the other change if you want to
          Hide
          Sam Marshall added a comment -

          Thanks for confirming that, Jay. I don't think anyone else would really need the different classes either, so I'm not going to bother adding them to the other patch (agree it didn't hurt to have them, though).

          And yes I know you submitted the issue and code originally, but you can't be expected to know that somebody else did similar. But it was me who submitted them both for integration, so I definitely should have noticed! I think my memory must be suffering due to old age...

          Show
          Sam Marshall added a comment - Thanks for confirming that, Jay. I don't think anyone else would really need the different classes either, so I'm not going to bother adding them to the other patch (agree it didn't hurt to have them, though). And yes I know you submitted the issue and code originally, but you can't be expected to know that somebody else did similar. But it was me who submitted them both for integration, so I definitely should have noticed! I think my memory must be suffering due to old age...
          Hide
          Dan Poltawski added a comment -

          Thanks Sam/Jay/Raj, i've reverted this and reopening.

          Show
          Dan Poltawski added a comment - Thanks Sam/Jay/Raj, i've reverted this and reopening.
          Hide
          Dan Poltawski added a comment -

          Closing as a duplicate of MDL-36991, cheers!

          Show
          Dan Poltawski added a comment - Closing as a duplicate of MDL-36991 , cheers!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: