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.

Sign up for GitHub

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

Automatic DegreeSite creation upon Degree entity creation #25

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

Automatic DegreeSite creation upon Degree entity creation #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
496fc81
Connect the creation of the degree site with the creation of the degr…
Apr 2, 2015
161c82f
Define canPostGroup. Create the folder when it doesnt exists.
Apr 20, 2015
50ef9bf
Merge pull request #2 from JNPA/DegreeSiteCreation
sanavarali Apr 20, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions src/main/java/org/fenixedu/learning/domain/degree/DegreeSite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,46 @@
*/
package org.fenixedu.learning.domain.degree;

import static com.google.common.base.Joiner.on;
import static com.google.common.base.Preconditions.checkNotNull;

import java.util.Locale;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Stream;

import org.fenixedu.academic.domain.Degree;
import org.fenixedu.academic.domain.accessControl.CoordinatorGroup;
import org.fenixedu.bennu.core.domain.Bennu;
import org.fenixedu.bennu.core.groups.DynamicGroup;
import org.fenixedu.bennu.core.i18n.BundleUtil;
import org.fenixedu.bennu.core.util.CoreConfiguration;
import org.fenixedu.bennu.portal.domain.MenuContainer;
import org.fenixedu.bennu.portal.domain.PortalConfiguration;
import org.fenixedu.cms.domain.CMSFolder;
import org.fenixedu.cms.domain.Category;
import org.fenixedu.cms.domain.wraps.Wrap;
import org.fenixedu.commons.i18n.LocalizedString;
import pt.ist.fenixframework.DomainObject;
import org.joda.time.DateTime;

import java.util.Locale;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Stream;
import pt.ist.fenixframework.DomainObject;

public class DegreeSite extends DegreeSite_Base {

public DegreeSite(Degree degree) {
super();
checkNotNull(degree);
setDegree(degree);

setFolder(folderForPath(PortalConfiguration.getInstance().getMenu(), "degrees"));
setSlug(on("-").join(degree.getSigla(), degree.getExternalId()));

setCreationDate(new DateTime());
setCanAdminGroup(DynamicGroup.get("managers"));
setCanPostGroup(CoordinatorGroup.get(degree));

setPublished(true);
setBennu(Bennu.getInstance());
degree.setSiteUrl(getFullUrl());
}

@Override
Expand Down Expand Up @@ -72,4 +94,19 @@ public Stream<Wrap> getCategoriesToShow() {
return Stream.of(categoryForSlug("announcement")).filter(Objects::nonNull).map(Category::makeWrap);
}

private CMSFolder folderForPath(MenuContainer parent, String path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that folders are optional in the CMS, it does not make sense to blow up if that particular folder is not found.

One might want not to organize degree sites in folders (or even set them up in a custom, on different path), so I think it would just be better to do nothing in this case.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, like ExecutionCourseSite example, if parent folder not found, now is created a new one.


LocalizedString.Builder description = new LocalizedString.Builder();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simply use BundleUtil.getLocalizedString, it has the same effect as this.

CoreConfiguration
.supportedLocales()
.stream()
.forEach(
l -> description.with(l,
BundleUtil.getString("resources.FenixEduLearningResources", l, "label.degree.folder.description")));

return parent.getOrderedChild().stream().filter(item -> item.getPath().equals(path))
.map(item -> item.getAsMenuFunctionality().getCmsFolder()).findAny()
.orElseGet(() -> new CMSFolder(parent, path, description.build()));
}

}
15 changes: 10 additions & 5 deletions src/main/java/org/fenixedu/learning/domain/executionCourse/ExecutionCourseSite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,12 @@
import org.fenixedu.academic.domain.accessControl.StudentSharingDegreeOfCompetenceOfExecutionCourseGroup;
import org.fenixedu.academic.domain.accessControl.StudentSharingDegreeOfExecutionCourseGroup;
import org.fenixedu.academic.domain.accessControl.TeacherGroup;
import org.fenixedu.academic.domain.accessControl.TeacherResponsibleOfExecutionCourseGroup;
import org.fenixedu.bennu.core.domain.Bennu;
import org.fenixedu.bennu.core.groups.AnyoneGroup;
import org.fenixedu.bennu.core.groups.DynamicGroup;
import org.fenixedu.bennu.core.groups.Group;
import org.fenixedu.bennu.core.groups.LoggedGroup;
import org.fenixedu.bennu.core.groups.NobodyGroup;
import org.fenixedu.bennu.core.i18n.BundleUtil;
import org.fenixedu.bennu.core.util.CoreConfiguration;
import org.fenixedu.bennu.portal.domain.MenuContainer;
Expand All @@ -44,6 +45,7 @@
import org.fenixedu.cms.domain.Category;
import org.fenixedu.cms.domain.wraps.Wrap;
import org.fenixedu.commons.i18n.LocalizedString;
import org.joda.time.DateTime;

import pt.ist.fenixframework.Atomic;
import pt.ist.fenixframework.DomainObject;
Expand All @@ -56,13 +58,16 @@ public class ExecutionCourseSite extends ExecutionCourseSite_Base {
public ExecutionCourseSite(ExecutionCourse executionCourse) {
checkNotNull(executionCourse);
setExecutionCourse(executionCourse);
setPublished(true);

setFolder(folderForPath(PortalConfiguration.getInstance().getMenu(), "courses"));
setSlug(on("-").join(getExecutionCourse().getSigla(), getExecutionCourse().getExternalId()));
setCanAdminGroup(NobodyGroup.get());
setCanPostGroup(NobodyGroup.get());
setBennu(Bennu.getInstance());

setCreationDate(new DateTime());
Copy link
Contributor

Choose a reason for hiding this comment

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

Creation date shouldn't be set here.

Copy link

Choose a reason for hiding this comment

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

This comment leaves everything to be guessed, again.
I do not know why it shouldn't be set here. The creation time-stamp being set inside the constructor of a certain entity is a simple, natural and widely accepted practice. So natural it even feels silly having to justify something like this.
Nevertheless, if a reasonable argument and an alternative are provided we will gladly indulge you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Creation date is already set at the Site constructor. Calling the super() method should set that for you, so there is no need to set it here again.

setCanAdminGroup(DynamicGroup.get("managers"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware that setting the canAdminGroup does not set the canPostGroup (which by default is initialized with a UserGroup of the current user).

Copy link
Author

Choose a reason for hiding this comment

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

Changed the "canPostGroup" to ExecutionCourse responsibles.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have some concerns settings this to manager. All the managers will be have access to any execution course page.

setCanPostGroup(TeacherResponsibleOfExecutionCourseGroup.get(executionCourse));
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now this has no effect, and may cause problems. We don't want to give teacher this interface right now. Adding this at this junction should be done on another place and not here.

Copy link

Choose a reason for hiding this comment

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

I understand your comment refers to L67.
Indeed this is not the correct behavior in my view. Given the frailties of CMS and especially its lack of proper access control to support this particular use case, it is not a good idea to give PostingAccess to the teachers (responsible or not) of a certain course. Since we provide a separate mechanism to manage the CourseSite, through the Teacher portal, we should refrain from allowing the teachers to also manage the same site through the CMS portal.
That said, if this group must be initialized at all, it should be with the NobodyGroup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, for now lets put it at Nobody, and when we have the access control on CMS revisit this problem. What I would suggest doing on your installations is setting this to the appropriate access control using signals or scripts.


setPublished(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this changed place.

Copy link

Choose a reason for hiding this comment

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

As before, I'll guess you are referring to L69. Line 69 changing places must have been simply the consequence of code editing. Despite always being an interesting line number, it's barely even worth mentioning, and certainly not worth raising an issue around it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, no problem. I wasn't raising an issue, I was just wondering why.

setBennu(Bennu.getInstance());
Copy link
Contributor

Choose a reason for hiding this comment

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

This line must be back ported to CMS, most likely this doesn't make sense to be made on this place.

Copy link

Choose a reason for hiding this comment

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

Which line? You are referring to a whole diff block.
Should be moved where to CMS? Which entity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only Line 70. Attaching the instance to Bennu should be done within the CMS. This is a bug, and should be corrected at CMS. But for now lets leave it here, because this isn't fixed yet on the other module

executionCourse.setSiteUrl(getFullUrl());
}

Expand Down
6 changes: 5 additions & 1 deletion src/main/java/org/fenixedu/learning/servlets/FenixEduLearningContextListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import javax.servlet.ServletContextListener;
import javax.servlet.annotation.WebListener;

import org.fenixedu.academic.domain.Degree;
import org.fenixedu.academic.domain.ExecutionCourse;
import org.fenixedu.academic.domain.Summary;
import org.fenixedu.academic.domain.thesis.Thesis;
Expand All @@ -35,6 +36,7 @@
import org.fenixedu.cms.domain.Post;
import org.fenixedu.commons.i18n.I18N;
import org.fenixedu.commons.i18n.LocalizedString;
import org.fenixedu.learning.domain.degree.DegreeSiteListener;
import org.fenixedu.learning.domain.executionCourse.ExecutionCourseListener;
import org.fenixedu.learning.domain.executionCourse.SummaryListener;
import org.joda.time.DateTime;
Expand Down Expand Up @@ -62,10 +64,12 @@ public void contextInitialized(ServletContextEvent sce) {
Signal.register(Summary.EDIT_SIGNAL, (DomainObjectEvent<Summary> event) -> {
SummaryListener.updatePost(event.getInstance().getPost(), event.getInstance());
});

Signal.register(ExecutionCourse.CREATED_SIGNAL, (DomainObjectEvent<ExecutionCourse> event) -> {
ExecutionCourseListener.create(event.getInstance());
});
Signal.register(Degree.CREATED_SIGNAL, (DomainObjectEvent<Degree> event) -> {
DegreeSiteListener.create(event.getInstance());
});
Signal.register(PublishMarks.MARKS_PUBLISHED_SIGNAL, FenixEduLearningContextListener::handleMarksPublishment);
Signal.register(Thesis.PROPOSAL_APPROVED_SIGNAL, FenixEduLearningContextListener::handleThesisProposalApproval);
FenixFramework.getDomainModel().registerDeletionListener(ExecutionCourse.class, (executionCourse) -> {
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/resources/FenixEduLearningResources.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ degree.course.title=Course
label.title=Title
Inquiries=
label.course.folder.description=Folder for course sites
label.degree.folder.description=Folder for degree sites
label.page.not.found.title=Oops, we couldn't find this page
label.page.not.found=The page you are visiting can't be found on our server
label.back.to=&larr; Back to
Expand Down
3 changes: 2 additions & 1 deletion src/main/resources/resources/FenixEduLearningResources_en.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ label.evaluations=Evaluations
action.save=Save
label.title=Title
label.groups=Groups
label.course.folder.description=folder for course sites
label.course.folder.description=Folder for course sites
label.degree.folder.description=Folder for degree sites
label.latestAnnouncements = Latest Announcements
label.senior.lecture=Senior Lecturer
3 changes: 2 additions & 1 deletion src/main/resources/resources/FenixEduLearningResources_pt.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ label.link=Link
label.name=Nome
label.inquiriesResults=Resultados dos Inqueritos
label.groups=Grupos
label.course.folder.description=Pasta para paginas das cadeiras
label.course.folder.description=Pasta para as p�ginas das cadeiras
label.degree.folder.description=Pasta para as p�ginas dos cursos
label.no.grades=As notas ainda n�o foram publicadas.
label.page.not.found.title=Oops, n�o encontramos esta pagina
label.page.not.found=A pagina que est� a visitar n�o foi encontrada no servidor.
Expand Down