Skip to content

Conversation

@WaelAlahamdi
Copy link

Description

The Observer Role is a new property added to unit-level roles (unit_roles table) in the Doubtfire API. It enables read-only access for tutors or convenors who are granted observer-level privileges. Observers can view information but cannot perform any actions that modify data.

  • Added observer:boolean column to the unit_roles table via migration (20250803223033_add_observer_to_unit_roles.rb).
  • Updated app/helpers/authorisation_helpers.rb to enforce read-only access when observer is true.
  • Created unit tests in test/helpers/authorisation_helpers_test.rb to validate observer behavior.
  • Updated db/schema.rb to reflect the migration changes.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Added unit tests verifying:
    • Observer can perform :get actions.
    • Observer cannot perform non-GET actions.
  • Ran local tests:
    2 runs, 2 assertions, 0 failures, 0 errors
observer-test-pass

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if appropriate
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have created or extended unit tests to address my new additions
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

If you have any questions, please contact @macite or @jakerenzella.

Copy link

@JosephKS10 JosephKS10 left a comment

Choose a reason for hiding this comment

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

@ibi420
Copy link

ibi420 commented Sep 1, 2025

Hello Wael,

I’ve reviewed your changes and can confirm they’re stable and well-written. From my understanding, this task was to create a role with read-only access in OnTrack. I tested this using Postman and verified that change attempts are correctly rejected when the role is set to true, which aligns with the expected behaviour. The tests you’ve written also confirm these findings. Good work on changing the base branch!

Thank you for the opportunity to review your work, and good job on this task.

Copy link

@chelaz1234 chelaz1234 left a comment

Choose a reason for hiding this comment

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

Hi Wael,

As Ibi mentioned, I also validated this through Postman and confirmed that modification attempts are properly blocked when the observer role is enabled. That means the observer logic is indeed being triggered as expected, which is great to see.

I also want to highlight some positives in this work:

Preventing observer users from making write actions is an important access-control improvement and aligns well with least-privilege principles.

You included a migration that’s clean and safe, with a sensible default.

Tests were added alongside the change rather than leaving it untested. Even if the coverage doesn’t fully exercise every path, pairing implementation with tests is a very good practice.

Overall, this is a solid step forward in tightening role-based permissions. Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants