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

SAK-50915 LTI send instructor role for DA user with instructor perms #13238

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ottenhoff
Copy link
Contributor

No description provided.

@ottenhoff ottenhoff requested a review from csev January 25, 2025 15:11
@csev
Copy link
Contributor

csev commented Jan 25, 2025

@ottenhoff I don't understand. You are deleting a feature that was put there on purpose. Those role maps are there for tools with weird situations. We should just fix the role maps - they don't get in the way for the non-DA users.

@ottenhoff
Copy link
Contributor Author

@ottenhoff I don't understand. You are deleting a feature that was put there on purpose. Those role maps are there for tools with weird situations. We should just fix the role maps - they don't get in the way for the non-DA users.

If you opened your code in an IDE, you would see some nice friendly suggestions that your role maps (in this specific method) are not doing a thing. You pass the role maps into this helper method and then you don't use them.

Map<String, String> propRoleMap = convertOutboundRoleMapPropToMap(
ServerConfigurationService.getString(LTI_OUTBOUND_ROLE_MAP)
);
Map<String, String> defaultRoleMap = convertOutboundRoleMapPropToMap(LTI_OUTBOUND_ROLE_MAP_DEFAULT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you dont use these. this method seems to be about returning a Sakai role only.

AuthzGroup realm = ComponentManager.get(AuthzGroupService.class).getAuthzGroup(realmId);
if (realm != null) {
role = realm.getUserRole(user.getId());

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I see what you are doing. White space though :)

@csev
Copy link
Contributor

csev commented Jan 25, 2025

Thanks -Always love to delete code that does nothing. I see what you added and like it. Noted some whitespace :) Approved.

@csev
Copy link
Contributor

csev commented Jan 25, 2025

Yup @ottenhoff I need to learn an IDE. I just fear the setup and learning curve.

@ottenhoff
Copy link
Contributor Author

ottenhoff commented Jan 25, 2025 via email

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.

2 participants