-
-
Notifications
You must be signed in to change notification settings - Fork 967
SAK-51224 conversations Create gb item when publishing topic #13582
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
base: master
Are you sure you want to change the base?
Conversation
https://sakaiproject.atlassian.net/browse/SAK-51224 Rather than add a grading item when the topic is published, this PR presents an error banner on the topic display if certain conditions are present: topic.graded = true and topic.gradingItemId is undefined. The user is presented with a banner stating that they need to edit the topic and setup a grading item association, probably because the topic was imported from an archive. I've taken this approach because I would have needed to duplicate some grading item data in the source topic and the xml archive to be able to automagically create the same grading item. I didn't want to pollute the topic entity just for that purpose.
@@ -145,6 +145,9 @@ public class ConversationsTopic implements PersistableEntity<String> { | |||
@Column(name = "UPVOTES") | |||
private Integer upvotes = 0; | |||
|
|||
@Column(name = "GRADED") | |||
private Boolean graded = Boolean.FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the script on the sakai-reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need a boolean at all? you have a GRADING_ITEM_ID Long!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it seemed really odd dumping that grading item id into the xml and using it as a switch to tell the target system to generate a new grading item with a different id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate that GRADED boolean too, but passing an id from another system is just pollution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont understand! just convert it into a boolean in the XML gen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I'll set gradingItemId in the xml to -1 or something to indicate that it was graded and that we need a new grading item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that's a less-ideal solution, don't do it! not a big deal IMO ... was trying to save you from creating conversion scripts and adding complexity ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to either. It just feels a bit janky using gradingItemId like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it sounds like you have some sort of automated method that moves database fields into XML .... but obviously in normal code, it's perfectly fine to if (gradingItemId > 0)
instead of if (grading)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've spent a few hours working up a solution that uses -1 as a signifier that a grading item needs to be created when the user wants to publish an imported topic. It feels a lot more brittle logic wise than just setting a graded flag without setting -1 as the gradingItemId and updating multiple places to add special handling for that case.
conversations/impl/src/test/org/sakaiproject/conversations/impl/ConversationsServiceTests.java
Outdated
Show resolved
Hide resolved
webcomponents/tool/src/main/frontend/packages/sakai-conversations/test/i18n.js
Outdated
Show resolved
Hide resolved
Co-authored-by: bbbbgarcia <[email protected]>
…ons/test/i18n.js Co-authored-by: bbbbgarcia <[email protected]>
…l/ConversationsServiceTests.java Co-authored-by: bbbbgarcia <[email protected]>
https://sakaiproject.atlassian.net/browse/SAK-51224
Rather than add a grading item when the topic is published, this PR presents an error banner on the topic display if certain conditions are present: topic.graded = true and topic.gradingItemId is undefined. The user is presented with a banner stating that they need to edit the topic and setup a grading item association, probably because the topic was imported from an archive.
I've taken this approach because I would have needed to duplicate some grading item data in the source topic and the xml archive to be able to automagically create the same grading item. I didn't want to pollute the topic entity just for that purpose.