Moodle

Cannot change roles on sticky blocks

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 1.8
  • Fix Version/s: 1.8.3, 1.9
  • Component/s: Blocks
  • Labels:
    None
  • Environment:
    moodle 1.8 on linux
  • Database:
    MySQL
  • Affected Branches:
    MOODLE_18_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE, MOODLE_19_STABLE

Description

On the sticky blocks admin page, the role icon has an empty "contextid" parameters. So it is not possible to generate sticky blocks for teachers only.

Brief analysis:
It looks like validate_context fails because ist gets the id of the "block_pinned" table and not the id of the corresponding record in table block.

Issue Links

This issue will help resolve:
MDL-11814 MDL-10573 breaks sticky blocks Major Closed
This issue has been marked as being related by:
MDL-10977 Sticky blocks should be integrated better with other blocks (roles etc) Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Mike Churchward added a comment - - edited

This appears to be caused by an incorrect 'contextid' on the 'assign roles' icon for sticky blocks. It seems to always pick a context id associated with a block on the main site page, rather than the actual sticky block...

Investigating further...

Ah. I see why. The pinned blocks still use 'CONTEXT_BLOCK', but are stored in a separate block table than blocks. Blocks are in 'block_instance', while sticky blocks are in 'block_pinned'. This means that the 'get_context_instance' function is getting records with an instance id referring to the wrong table. Worse yet, they may actually create a context id record for the wrong block!

Note that the original complaint said that icon had an empty context id. Unfortunately, it may not be empty if it finds a regular block entry with the same id as the sticky block.

Show
Mike Churchward added a comment - - edited This appears to be caused by an incorrect 'contextid' on the 'assign roles' icon for sticky blocks. It seems to always pick a context id associated with a block on the main site page, rather than the actual sticky block... Investigating further... Ah. I see why. The pinned blocks still use 'CONTEXT_BLOCK', but are stored in a separate block table than blocks. Blocks are in 'block_instance', while sticky blocks are in 'block_pinned'. This means that the 'get_context_instance' function is getting records with an instance id referring to the wrong table. Worse yet, they may actually create a context id record for the wrong block! Note that the original complaint said that icon had an empty context id. Unfortunately, it may not be empty if it finds a regular block entry with the same id as the sticky block.
Hide
Permalink
Petr Škoda (skodak) added a comment -

I think we should create a new context for pinned blocks....

Show
Petr Škoda (skodak) added a comment - I think we should create a new context for pinned blocks....
Hide
Permalink
Mike Churchward added a comment -

...and just so we understand the issues here, if I DO assign a role or override a role to a sticky block, I actually may be assigning or overriding to a completely different block on a different page.

Show
Mike Churchward added a comment - ...and just so we understand the issues here, if I DO assign a role or override a role to a sticky block, I actually may be assigning or overriding to a completely different block on a different page.
Hide
Permalink
Mike Churchward added a comment - - edited

I don't think introducing a new CONTEXT will work. Context's are element specific - BLOCK, COURSE, etc. A block's pinned nature is just a state - that is a 'pinned' block is still a block. How would we add new capabilities to blocks? Would we need to add capabilities for both CONTEXT_BLOCK and CONTEXT_BLOCKPINNED?

I think the better fix is to get rid of the 'block_pinned' table and add a 'pinned' field to the 'block_instance' table.

Show
Mike Churchward added a comment - - edited I don't think introducing a new CONTEXT will work. Context's are element specific - BLOCK, COURSE, etc. A block's pinned nature is just a state - that is a 'pinned' block is still a block. How would we add new capabilities to blocks? Would we need to add capabilities for both CONTEXT_BLOCK and CONTEXT_BLOCKPINNED? I think the better fix is to get rid of the 'block_pinned' table and add a 'pinned' field to the 'block_instance' table.
Hide
Permalink
Martin Dougiamas added a comment -

Hmm ....

I do agree that block_pinned could be better re-implemented with a single new column in block_instance, just for increased simplicity alone.

About the contexts, it's a bit tricky to think about since the one "instance" appears site-wide within various other contexts, and so may inherit different permissions from the various parent contexts. Do we process all this like a normal block, effectively treating each place it appears as a sort of virtual instance?

Or are pinned blocks an exception to heirarchy rules, with their own context under SYSTEM level?

Needs some thinking about.

Show
Martin Dougiamas added a comment - Hmm .... I do agree that block_pinned could be better re-implemented with a single new column in block_instance, just for increased simplicity alone. About the contexts, it's a bit tricky to think about since the one "instance" appears site-wide within various other contexts, and so may inherit different permissions from the various parent contexts. Do we process all this like a normal block, effectively treating each place it appears as a sort of virtual instance? Or are pinned blocks an exception to heirarchy rules, with their own context under SYSTEM level? Needs some thinking about.
Hide
Permalink
Martin Dougiamas added a comment -

In the meantime I've removed the roles assign button from ever showing on sticky/pinned blocks.

I'll file a new bug for improving sticky blocks.

Show
Martin Dougiamas added a comment - In the meantime I've removed the roles assign button from ever showing on sticky/pinned blocks. I'll file a new bug for improving sticky blocks.
Hide
Permalink
Mike Churchward added a comment -

From what I can tell, the only reason that pinned blocks are in a separate table is to manage the positions easier. If that's true, they could be moved to the same table and just rewrite the code to deal with position issues.

Show
Mike Churchward added a comment - From what I can tell, the only reason that pinned blocks are in a separate table is to manage the positions easier. If that's true, they could be moved to the same table and just rewrite the code to deal with position issues.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.