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-50756 Tags new endpoint to get assignments #13107

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

Conversation

jumarub
Copy link
Contributor

@jumarub jumarub commented Dec 12, 2024

@jesusmmp jesusmmp changed the title SAK-50756 SAK-50756 New Endpoint getTagsByAssignmentId Dec 12, 2024
@bgarciaentornos bgarciaentornos changed the title SAK-50756 New Endpoint getTagsByAssignmentId SAK-50756 Tag Service: new Endpoint getTagsByAssignmentId Dec 12, 2024
@ern ern changed the title SAK-50756 Tag Service: new Endpoint getTagsByAssignmentId SAK-50756 Tags new endpoint to get assignments Dec 12, 2024
private SessionManager sessionManager = (SessionManager) ComponentManager.get("org.sakaiproject.tool.api.SessionManager");
private SessionManager sessionManager = (SessionManager) ComponentManager.get("org.sakaiproject.tool.api.SessionManager");

private AssignmentService assignmentService = (AssignmentService) ComponentManager.get("org.sakaiproject.assignment.api.AssignmentService");;
Copy link
Contributor

Choose a reason for hiding this comment

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

It propbably makes sense to use the entity manager vs the assignment service directly as your only retrieving assignments.
Also it would be ideal to migrate away from using the ComponentManager directly

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 changed the approach deleting the use of assignment service, the endpoint could be used for other tools.

String assignmentId = wp.getString("assignmentId");

Assignment assignment = assignmentService.getAssignment(assignmentId);
String siteId = assignment.getContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

SiteId should be a param , this can be called from other tools too. Method should be renamed too

@jumarub jumarub force-pushed the SAK-50756 branch 2 times, most recently from e5ee466 to 49096d1 Compare December 13, 2024 12:19
@jumarub jumarub force-pushed the SAK-50756 branch 2 times, most recently from 083603d to d7fd8d3 Compare December 13, 2024 12:39
Comment on lines 220 to 229
try {
WrappedParams wp = new WrappedParams(params);
String itemId = wp.getString("itemId");
String siteId = wp.getString("siteId");

List<Tag> tagList = tagService().getAssociatedTagsForItem(siteId, itemId);

return buildtagJsonOject(tagList, tagList.size());
} catch (Exception e) {
log.error("Error calling getTagsByItemId:", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try {
WrappedParams wp = new WrappedParams(params);
String itemId = wp.getString("itemId");
String siteId = wp.getString("siteId");
List<Tag> tagList = tagService().getAssociatedTagsForItem(siteId, itemId);
return buildtagJsonOject(tagList, tagList.size());
} catch (Exception e) {
log.error("Error calling getTagsByItemId:", e);
WrappedParams wp = new WrappedParams(params);
String itemId = wp.getString("itemId");
String siteId = wp.getString("siteId");
try {
List<Tag> tagList = tagService().getAssociatedTagsForItem(siteId, itemId);
return buildtagJsonOject(tagList, tagList.size());
} catch (Exception e) {
log.warn("Could not get tag with itemId={} in site={}, {}", itemId, siteId, e);

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