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

Add workspace root content type #3749

Merged
merged 1 commit into from Dec 13, 2017
Merged

Add workspace root content type #3749

merged 1 commit into from Dec 13, 2017

Conversation

raphael-s
Copy link
Contributor

@raphael-s raphael-s commented Dec 7, 2017

Implements a new content type "WorkspaceRoot" which is the folderish parent for workspaces. (Workspace content type will be added later)

closes #3675

@raphael-s raphael-s self-assigned this Dec 7, 2017
@raphael-s raphael-s requested a review from a team December 7, 2017 16:23
@raphael-s raphael-s changed the title Implement workspace module (OGIP 17) Add workspace root content type Dec 7, 2017
@raphael-s raphael-s force-pushed the rs-3675-WorkspaceRoot branch 3 times, most recently from d5fbff2 to d5aa211 Compare December 7, 2017 17:18
<!-- enabled behaviors -->
<property name="behaviors">
<element value="opengever.tabbedview.interfaces.ITabbedViewEnabled" />
<element value="opengever.sharing.behaviors.IDossier" />
Copy link
Member

Choose a reason for hiding this comment

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

Is that really necessary, as soon as I know the workspace part should habe their own roles and permissions.

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 used the xml of opengever.repository.repositoryroot as a base for this. I removed the behaviour now.

<alias from="view" to="@@view" />

<!-- Actions -->

Copy link
Member

Choose a reason for hiding this comment

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

✂️


def __call__(self):
self.install_upgrade_profile()
self.update_workflow_security(
Copy link
Member

Choose a reason for hiding this comment

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

what ??? 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clicked the button in lawgiver, I thought this would be necessary since I added a new permission. Seems like it is actually not needed, I removed the workflow update completely.

@@ -0,0 +1,716 @@
<?xml version='1.0' encoding='UTF-8'?>
Copy link
Member

Choose a reason for hiding this comment

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

??? 😞

--> 🗑

@@ -0,0 +1,401 @@
<?xml version='1.0' encoding='UTF-8'?>
Copy link
Member

Choose a reason for hiding this comment

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

--> 🗑

"_path": "teamraeume",
"_type": "opengever.workspace.root",
"title_de": "Teamräume",
"title_fr": "Teamräume",
Copy link
Member

Choose a reason for hiding this comment

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

ask niklaus for a meaningful translation.

"_type": "opengever.workspace.root",
"title_de": "Teamräume",
"title_fr": "Teamräume",
"_transitions": "publish"
Copy link
Member

Choose a reason for hiding this comment

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

is there a publish transition ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is. Have a look at the item above.

It works, just tested it. The item is published after its creation.

def create_workspaces(self):
self.workspace_root = self.register('workspace_root', create(
Builder('workspace_root').having(title_de=u'Teamr\xe4ume',
title_fr=u'Teamr\xe4ume')))
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -0,0 +1,6 @@
<configure
Copy link
Member

Choose a reason for hiding this comment

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

We do not have a content subdirectory in GEVER, i would move all that to opengever/workspace

<property name="description" i18n:translate="" />
<property name="icon_expr" />
<property name="allow_discussion">False</property>
<property name="global_allow">True</property>
Copy link
Member

Choose a reason for hiding this comment

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

-> False: you've already added it to the allowed-types on Plone Site.

<property name="default_view">tabbed_view</property>
<property name="default_view_fallback">False</property>
<property name="view_methods">
<element value="view" />
Copy link
Member

Choose a reason for hiding this comment

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

✂️ I think we only want a tabbedview here.
Having two values here probably lets the layout-menu pop up; we don't want that in GEVER.

@@ -805,6 +806,11 @@ def create_committee(self, title, repository_folder, group_id,
group_id=group_id))
return committee

def create_workspaces(self):
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this to create_workspace_root.

Creating the workspaces should be separated and executed by another user in another freezed hour.



@adapter(IFolderish, IDefaultBrowserLayer, IDexterityFTI)
class AddView(DefaultAddView):
Copy link
Member

Choose a reason for hiding this comment

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

This view is not registered.
How do we test that?

need any specific schema fields """


alsoProvides(IWorkspaceRootSchema, IFormFieldProvider)
Copy link
Member

Choose a reason for hiding this comment

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

In GEVER we prefer to use the decorator style.
So please use

@provider(IFormFieldProvider)
class IWorkspaceRootSchema(model.Schema):
    …

See https://github.com/4teamwork/opengever.core/blob/master/opengever/meeting/proposaltemplate.py#L14
I'm aware that there are places where this is not yet converted.

@raphael-s raphael-s force-pushed the rs-3675-WorkspaceRoot branch 5 times, most recently from b499001 to 99c4e91 Compare December 8, 2017 11:36
@raphael-s
Copy link
Contributor Author

@jone @phgross I implemented your requested changes. Is it okay now?

Copy link
Member

@jone jone left a comment

Choose a reason for hiding this comment

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

Bitte noch Übersetzungen den Titel des FTI auf Deutsch übersetzen.

Ansonsten siehts gut aus 👍

@jone
Copy link
Member

jone commented Dec 11, 2017

Der Titel fehlt / ist kaputt.
bildschirmfoto 2017-12-11 um 13 36 04

@jone
Copy link
Member

jone commented Dec 11, 2017

Bitte Standard-Workflow entfernen: der Typ soll momentan keinen Workflow haben, das kommt später.
bildschirmfoto 2017-12-11 um 13 37 48

need any specific schema fields """


class WorkspaceRoot(Item):
Copy link
Member

Choose a reason for hiding this comment

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

Funktioniert die TranslatedTitle Funktionalität wirklich wie gewünscht? Aus meiner Sicht müsste hier das TranslatedTitleMixin verwendet werden. 🤔

@raphael-s raphael-s force-pushed the rs-3675-WorkspaceRoot branch 4 times, most recently from ddc5cdb to e84fcec Compare December 12, 2017 09:28
@raphael-s
Copy link
Contributor Author

@jone I removed the default workflow and added the TranslatedTitleMixin the way @phgross suggested it, the title is now displayed correctly. i18n translations for the FTI are now available aswell.

@@ -231,6 +231,10 @@ msgstr "Amtswechsler umschalten"
msgid "Unlock unused repository prefixes."
msgstr "Freigabe von ungebrauchten Aktenzeichen Prefixen."

#: ./opengever/core/profiles/default/types/opengever.workspace.root.xml
msgid "WorkspaceRoot"
msgstr "Teamruäume"
Copy link
Member

Choose a reason for hiding this comment

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

typo

@@ -24,5 +24,12 @@
"title_de": "Kontakte",
"title_fr": "Contacts",
"_transitions": "publish"
},
{
"_path": "teamraeume",
Copy link
Member

Choose a reason for hiding this comment

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

Ich finde die ID nicht schön. Wie wärs mit workspaces?

def create_workspace_root(self):
self.workspace_root = self.register('workspace_root', create(
Builder('workspace_root').having(title_de=u'Teamr\xe4ume',
title_fr=u'Espace partag\xe9')))
Copy link
Member

Choose a reason for hiding this comment

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

Kann man hier die ID setzen? Wäre cool wenn wir die gleiche ID hätten wie im Example Content.

@raphael-s raphael-s force-pushed the rs-3675-WorkspaceRoot branch 2 times, most recently from 5797000 to c4fd8c9 Compare December 12, 2017 12:52
@raphael-s
Copy link
Contributor Author

@jone Done 👍

Copy link
Member

@jone jone left a comment

Choose a reason for hiding this comment

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

@raphael-s noch ein kleines Detail, ansonsten finde ichs gut.

class WorkspaceRoot(Item, TranslatedTitleMixin):
implements(IWorkspaceRoot)

Title = TranslatedTitleMixin.Title
Copy link
Member

Choose a reason for hiding this comment

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

@raphael-s mich hat schon vorher irritiert wie die Reihenfolge der vererbten Klasse ist; diese Zeile erklärts nun 😉

Ich schlage vor, dass du diese Zeile entfernst und dafür die Reihenfolge der Vererbten Superklassen umkehrst:

-class WorkspaceRoot(Item, TranslatedTitleMixin):
+class WorkspaceRoot(TranslatedTitleMixin, Item):

Die MRO funktioniert so, dass die Klassen links die Klassen rechts überschreiben.

Ich würde dies machen auch wenn die anderen Orte im GEVER nicht so sind. Weil das Umkehren der MRO ist eigentlich korrekt und es hat keine Seiteneffekte.

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 order as you requested, now the tests fail. Also the title is no longer translated. Are you sure that the override order is left to right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think I got it now 😄

But the tests are still failing... what am I doing wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jone The inheritance order has to be the way it was before because Item does some magic which gets broken by changing the order. See -> #1394

@raphael-s raphael-s force-pushed the rs-3675-WorkspaceRoot branch 4 times, most recently from 6c5543d to e357fc7 Compare December 13, 2017 14:41
Copy link
Member

@jone jone left a comment

Choose a reason for hiding this comment

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

Ok then; please rebase 😉

@raphael-s raphael-s merged commit 1d13b02 into master Dec 13, 2017
@raphael-s raphael-s deleted the rs-3675-WorkspaceRoot branch December 13, 2017 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OGIP 17: New FTI workspace root
3 participants