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

Untranslated role title in header of the role GUI #4854

Conversation

schmitz-ilias
Copy link
Contributor

@smeyer-ilias As discussed in #4676, this PR adds the untranslated role title in brackets to the header of role and role template GUIs, when the role (template) is autogenerated. Note that this required a tiny change to an OrgUnit class to retain its previous behavior, as well as a bunch of cs fixes in that class.

@mjansenDatabay mjansenDatabay added bugfix php Pull requests that update Php code labels Jul 26, 2022
@klees klees assigned smeyer-ilias and unassigned klees Aug 2, 2022
@klees
Copy link
Member

klees commented Aug 2, 2022

@smeyer-ilias This looks like a change in Services/AccessControl which should also be reviewed by @mstuder as maintainer of Stuff.

@smeyer-ilias smeyer-ilias self-requested a review August 10, 2022 13:46
@smeyer-ilias smeyer-ilias requested a review from mstuder August 10, 2022 13:47
@iljalukin
Copy link
Contributor

@mstuder Could you please take a look at this? It would be nice to have untranslated role displayed again for assigning the local role when creating registration codes.

@oliversamoila
Copy link

Hello everyone,
I have only limited hope that we will achieve a result in this way. Do we have another party who could be responsible for a review of the changes in staff?
Thank you all.

@alex40724
Copy link
Member

alex40724 commented Apr 28, 2023

The code changes are in two components:

Modules/OrgUnit (now maintained by @klees )
Services/AccessControl (now maintained by @kergomard)

@klees Since you removed your assignment, I will not add you again.

@alex40724 alex40724 removed the request for review from mstuder April 28, 2023 16:02
@alex40724 alex40724 assigned kergomard and unassigned smeyer-ilias Apr 28, 2023
@kergomard
Copy link
Contributor

kergomard commented May 2, 2023

Thanks @schmitz-ilias for the PR.
Ok, I understand the use-case for this change.
I have some doubts about the implementation though: Where does the change in OrgUnits surface? I'm not really familiar with the OrgUnits. Couldn't we just show the complete title there, too? My reasoning: I don't like the introduction of a new function for just one use-case even though right now we are using the "short form" of the Role Title in many places. I also don't think it is the right nomenclature. We are adding "Long" and "Short" willy-nilly all over the place, just because we can not rename the main function. Personally I would like to remove the exception for the OrgUnits and just use the "long form" everywhere.

Thanks and best,
@kergomard

@schmitz-ilias schmitz-ilias force-pushed the untranslated-role-title-in-header branch from 1046c76 to 9375219 Compare May 4, 2023 13:36
@schmitz-ilias schmitz-ilias changed the base branch from trunk to release_7 May 4, 2023 13:37
@schmitz-ilias
Copy link
Contributor Author

@tfamula and I had a closer look at ilOrgUnitStaffGUI, and it seems like ilOrgUnitStaffGUI::addOtherRolesToToolbar is an unused remnant of a previous refactoring in OrgUnits, so no OrgUnit or Staff view will be affected by this PR. Accordingly, I dropped getShortPresenationTitle.

Quick sidenote @klees: it looks like ilOrgUnitStaffGUI is at most passing on ilCtrl calls to other classes, and might not do anything at all. It is not needed in Staff as far as we can tell.

Lastly, this PR was originally intended to also be merged to R7, so I changed the target branch to reflect that. @kergomard , let me know if there is anything else.

(PS: Apologies to all who got notified about me pushing ~250 commits, I might have force-pushed and changed target branches in the wrong order?)

@kergomard
Copy link
Contributor

Thanks for the update @schmitz-ilias ! I will merge and cherry-pick to 8 and trunk.
Best,
@kergomard

@kergomard kergomard merged commit 034ca63 into ILIAS-eLearning:release_7 May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants