-
-
Notifications
You must be signed in to change notification settings - Fork 906
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-49285 msgcntr oracle restriction unique violated SYS_C00508178 #11916
base: master
Are you sure you want to change the base?
Conversation
boolean isNew = topic.getId() == null; | ||
|
||
topic.setModified(new Date()); | ||
if(userId != null){ | ||
topic.setModifiedBy(userId); | ||
} | ||
getHibernateTemplate().saveOrUpdate(topic); | ||
topic = getHibernateTemplate().merge(topic); |
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 think it is good idea to treat topic as final and simply create a new var called "mergedTopic", it is clearer about what happened.
forum = getHibernateTemplate().merge(forum); | ||
area = getHibernateTemplate().merge(area); | ||
|
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.
Same here use mergedForum and mergedArea
newItems.add(item); | ||
newItems = saveSynopticMsgcntrItems(newItems); |
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.
this code could be less confusing newItems is never used in the way that it is declared at the top of the method.
saveSynopticMsgcntrItems(Arrays.asList(item)); | ||
newItems.add(item); | ||
newItems = saveSynopticMsgcntrItems(newItems); |
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.
same as previous method
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.
@jesusmmp looks pretty good just a few minor suggestions
@jesusmmp are you interested in getting this merged? |
I'll fix this PR in the summer |
https://sakaiproject.atlassian.net/browse/SAK-49285