Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support inspecting sessions #765

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Support inspecting sessions #765

merged 7 commits into from
Oct 25, 2024

Conversation

mauritsvanrees
Copy link
Contributor

This refs https://github.com/syslabcom/scrum/issues/2738 and I will make a PR in daimler.oira that uses this.

This lays a basis for "inspecting" sessions. What I mean with that, is that you can see all edit forms of a session, but with the form elements disabled.

This is meant for allowing to inspect archived or locked sessions. Until now that is not completely possible, because you are not allowed to view various pages because you have no edit rights anymore. Most importantly: the identification and action plan phases.

In webhelpers.py this PR sets:

allow_inspecting_archived_sessions = False
allow_inspecting_locked_sessions = False

With these settings, the new can_inspect_session method returns the same value as can_edit_session, so by default nothing should actually change. I will set this differently in daimler.oira so we can check it there.

The first two commits could be left out if wanted. The first is an optimisation to use @cached_property, which I like. The second is a small refactoring of the survey_tree_data method, which was my first approach for this issue, but it turned out to not be needed. Could still be useful though.

@mauritsvanrees
Copy link
Contributor Author

Note that the parts that I touched did not really use z3c.form, so adapting updateWidgets or similar was not an option as far as I saw. I needed to change templates.

@reinhardt
Copy link
Contributor

@ale-rt I can give this a review if you're busy with other things. I'm in psychosocial headspace anyway.

Copy link
Contributor

@reinhardt reinhardt left a comment

Choose a reason for hiding this comment

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

I wonder whether we also need to hide the save button(s) in the risk_identification_custom template. I haven't tested it yet, and in our GpB use case there are no custom risks AFAIK, but for completeness' sake we may want to cover those as well, right?

return False
if session.is_locked and not self.allow_inspecting_locked_sessions:
return False
return True
Copy link
Member

Choose a reason for hiding this comment

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

I would return a default which is stricter (e.g., self.is_manager).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would contradict #2672. Non-manager GpB users are supposed to be able to inspect GpB sessions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree that non-managers should be able to inspect sessions.

Note that currently we set allow_inspecting_locked_sessions = True in the class, but where wanted in a project (like daimler.oira) it could be overridden to something like this (using pseudocode):

@property
def allow_inspecting_locked_sessions(self):
    if self.is_manager:
        return True
    if this is a psychosocial session and user is NOT a psychosocial manager:
        return False
    return True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I do a simpler version in the related daimler.oira PR.

Copy link
Member

Choose a reason for hiding this comment

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

I am still concerned by this default.
Like it is now, this needs an override in customers projects to fully have sense.
Anyway it's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reported the default wrong in my comment above. In this Euphorie PR we set allow_inspecting_locked_sessions = False (same for archived).

With those defaults, can_inspect_session gives the same value as can_edit_session. So with the default settings this PR changes nothing:

  • if you can edit a session you can inspect it
  • if you can't edit a session you can't inspect it

This PR just makes it possible for policy packages like daimler.oira to set their own defaults, and make them dynamic where needed.

That said: I merge.

@ale-rt
Copy link
Member

ale-rt commented Oct 15, 2024

The first is an optimisation to use @cached_property, which I like

However, it's important to note that there are cases where it may not be the best choice. The @cached_property decorator works per instance, while the alternative combo works per request.

In some cases, such as when checking a registry record, memoize_contextless might be a better fit.

That said, I wouldn't recommend diving into a deep analysis of the code just to switch decorators, nor would I want to hijack a PR that's focused on something else to make this change. I’d prefer if such a change were driven by a clear rationale beyond personal preference, especially if it offers a significant advantage.

@ale-rt
Copy link
Member

ale-rt commented Oct 15, 2024

@ale-rt I can give this a review if you're busy with other things. I'm in psychosocial headspace anyway.

Thanks a lot @reinhardt, feel free to jump :)
Your suggestion was very much appreciated!

Add methods `get_active_and_disabled_for_section` and `is_section_disabled`.
This makes it possible to override the logic of those parts.
By default this is the same as `can_edit_session`.
But you can influence it by allowing to inspect archived and/or locked sessions.
Use case would be to show an edit form with all fields in display mode instead of edit mode.
I first set out to do this for all individual form elements, but you can just disable an entire fieldset.
These were still active, also in a disabled fieldset.
So simply display the contents directly if we cannot edit.
@mauritsvanrees
Copy link
Contributor Author

I wonder whether we also need to hide the save button(s) in the risk_identification_custom template. I haven't tested it yet, and in our GpB use case there are no custom risks AFAIK, but for completeness' sake we may want to cover those as well, right?

Good point, thanks. I now handle custom risks as well.

@mauritsvanrees
Copy link
Contributor Author

That said, I wouldn't recommend diving into a deep analysis of the code just to switch decorators, nor would I want to hijack a PR that's focused on something else to make this change.

Okay, I have removed my commit with the many @cached_property changes and have rebased and force pushed.

@mauritsvanrees mauritsvanrees merged commit 8025cbd into main Oct 25, 2024
2 checks passed
@mauritsvanrees mauritsvanrees deleted the maurits-inspect-session branch October 25, 2024 10:12
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.

3 participants