Ticket #9057 (closed PLIP: fixed)

Opened 5 years ago

Last modified 6 months ago

Using a folderish object as default view for a folder makes it impossible to add contents to the object.

Reported by: smcmahon Owned by: vangheem
Priority: major Milestone: 5.0
Component: General Version: 4.3
Keywords: UI Cc: timo@…, mamico, pingviini, vangheem

Description

If you use a folderish object -- like a collection or a PFG form -- as the default view for a folder, it becomes impossible to add content to that object via the Plone UI. Even using folder_contents won't get you into a position to do it.

Change History

comment:1 Changed 5 years ago by hannosch

  • Component changed from Unknown to Infrastructure

comment:2 Changed 4 years ago by hpeteragitator

  • Priority changed from minor to major

this bug is still exists in plone 4

peter

p.s. i don't consider this a feature ;)

comment:3 Changed 4 years ago by kleist

see also #10953

comment:4 Changed 3 years ago by jessilfp

  • Keywords TuneUp added

comment:5 Changed 3 years ago by WouterVH

  • Status changed from new to closed
  • Resolution set to fixed

I've been bitten by this as well. Setting structural folders as default page is not the most common use case. But assuming you know what you are doing, it seems most plausible that content-items should be added to that folder.

in plone.app.contentmenu and plone.app.content the add_context is defined by the context_state.folder()

    def add_context(self):
        context_state = getMultiAdapter((self.context, self.request), name='plone_context_state')
        return context_state.folder()

while the context_state.folder() explicitly rules out the case of the default-page:

../plonenext-3.3/eggs/plone.app.layout-1.2.6-py2.4.egg/plone/app/layout/globals/context.py

135     def folder(self):
136         if self.is_structural_folder() and not self.is_default_page():
137             return aq_inner(self.context)
138         else:
139             return self.parent()

I fixed this in plone.app.content + plone.app.contentmenu by explictly checking for a structural_folder before calling context_state.folder():

    def add_context(self):
        context_state = getMultiAdapter((self.context, self.request), name='plone_context_state')
        if context_state.is_structural_folder():
            return self.context
        else:
            return context_state.folder()

fix in r46151 in plone.app.content + fix in r46152 in plone.app.contentmenu

comment:6 Changed 3 years ago by rossp

  • Status changed from closed to reopened
  • Resolution fixed deleted

r46153 introduces test failures in plone.app.contentmenu. See the commit I made just previous to yours, r38679.

I've reverted this commit in r46174 until the issue is fixed.

comment:7 Changed 3 years ago by kiorky

  • Keywords TuneUp,green-belt added; TuneUp removed

comment:8 Changed 3 years ago by encolpe

  • Status changed from reopened to new
  • Owner set to encolpe
  • Milestone changed from 3.3.x to 4.3

Reading the tests I discovered an hack for the collection type inside the code: in a default site the 'events' folder have a collection as default page and must have 'Event' in 'Add' items. 'collection' type is a structural type that will create a bug if we try to make an exception.

To fix this we should implement the same ui that for DisplayMenu. This can delay the bugfix to plone 4.2 or Plone 4.3.

comment:9 Changed 3 years ago by encolpe

  • Status changed from new to assigned

comment:10 Changed 3 years ago by timo

  • Cc timo@… added

comment:11 Changed 3 years ago by mamico

  • Cc mamico added

comment:12 Changed 2 years ago by thet

  • Owner changed from encolpe to thet
  • Status changed from assigned to new

comment:13 Changed 2 years ago by thet

  • Owner changed from thet to enclope

comment:14 Changed 2 years ago by thet

sorry, accidentally assigned to me.

comment:15 Changed 2 years ago by thet

  • Status changed from new to assigned
  • Owner changed from enclope to thet

comment:16 Changed 2 years ago by thet

i've merged enclope's changes into my forks here:

but i have to stop to work on it for now, since the time runs out and the changes are quite big. there is another issue: a menu to add content to any default page is presented, e.g. the front-page which isn't folderish in a default plone installation. that might be easy to fix.

maybe someone else takes this one here...

comment:17 Changed 2 years ago by thet

  • Owner thet deleted
  • Status changed from assigned to new

comment:18 Changed 2 years ago by amleczko

  • Keywords TuneUp,black-belt added; TuneUp,green-belt removed

comment:19 Changed 2 years ago by pingviini

  • Status changed from new to assigned
  • Owner set to pingviini

comment:20 follow-up: ↓ 27 Changed 2 years ago by pingviini

  • Status changed from assigned to closed
  • Resolution set to fixed

I forked thet's work and modified getMenuItems method a bit so that we won't get extra menu for content types which aren't folderish. My work can be found from  https://github.com/pingviini/plone.app.contentmenu/tree/pingviini-defaultpagefix.

Current implementation has one weak spot though. If you 1) allow folder to be set as a default page 2) add new folder to your site root 3) add new page inside new folder and set it folders default page 4) set new folder to site roots default page

-> you will get the folders default page as a site roots default view, but you'll lose 'add new...' and display menus.

However I think this is quite extreme case where you're doing something wrong in the first hand.

comment:21 Changed 2 years ago by pingviini

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:22 Changed 2 years ago by pingviini

  • Keywords TuneUp,black-belt,forcommit added; TuneUp,black-belt removed

comment:23 Changed 22 months ago by davisagli

  • Component changed from Infrastructure to General

comment:24 Changed 22 months ago by kleist

  • Status changed from reopened to confirmed
  • Keywords TuneUp, black-belt, forcommit added; TuneUp,black-belt,forcommit removed
  • Version set to 4.1

Three heroes (encolpe, thet, pingviini) having been working on this for almost a year. So is this really still an issue?

Using 4.3 coredev buildout, I created a collection in the root and made it default view. I could then add an item to the root without problem.

comment:25 Changed 21 months ago by pingviini

  • Cc pingviini added

I think this is still valid issue for all 4.x Plone versions. The example with collections is not though as new collections aren't normal folderish objects anymore since Plone 4.2.

Issue can be confirmed with PloneFormGen (and Plone 4.2). PFG didn't work with plone 4.3 coredev buildout so I'm not 100% sure about the situation, but unless any of the code which enclope, thet and I have done have been merged to master I'm quite sure this bug still exists.

My pull request to thets fork is still in the github unnoticed ( https://github.com/thet/plone.app.contentmenu/pull/1). I'm reluctant to merge someone else work, so I think someone should poke thet about this (I've already sent few reminders in github comments) :)

comment:26 Changed 21 months ago by encolpe

The main problem is a design issue more than a technical issue: if we allow folderish content to be a default view we have to find a solution to display menus for the folderish and its container.

We are waiting an answer from the UI Team on this.

comment:27 in reply to: ↑ 20 ; follow-up: ↓ 28 Changed 20 months ago by jenskl

Replying to pingviini:

I forked thet's work and modified getMenuItems method a bit so that we won't get extra menu for content types which aren't folderish. My work can be found from  https://github.com/pingviini/plone.app.contentmenu/tree/pingviini-defaultpagefix.

Current implementation has one weak spot though. If you 1) allow folder to be set as a default page 2) add new folder to your site root 3) add new page inside new folder and set it folders default page 4) set new folder to site roots default page

-> you will get the folders default page as a site roots default view, but you'll lose 'add new...' and display menus.

However I think this is quite extreme case where you're doing something wrong in the first hand.

I disagree. This is actually a common use case. Example? Say, you want to have your news folder to be the default start page/view of your website. Then you

  1. allow folder to be set as default view (default_page_types under site_properties)
  2. go to myplone.org/select_default_page and select the news folder

Then the not-so-intuitive behaviour begins ;)

  • If you go to myplone.org then, then you do not have an "Add..." menu
  • If you want to add a news item, you first have to click on "View" leading to myplone.org/news/aggregator where you have an "Add news items" menu again and also ending up correctly in the news folder (a very non-intuive way this is)
  • Very puzzling: If you are on myplone.org, then "Contents" leads to /news/folder_contents, whereas "View" leads to /news/aggregator. And /news/ and /news/aggregator then have different "Add..." menus: /news/ lets you add items to the main plone top lever folder (/), whereas /news/aggregator lets add you only news to "news" folder (what you want).

That is, due to this bug, there is no easy way that is free of side effects which allows you to select your news folder as default start page of your website.

comment:28 in reply to: ↑ 27 Changed 19 months ago by pingviini

Replying to jenskl:

Replying to pingviini:

I forked thet's work and modified getMenuItems method a bit so that we won't get extra menu for content types which aren't folderish. My work can be found from  https://github.com/pingviini/plone.app.contentmenu/tree/pingviini-defaultpagefix.

Current implementation has one weak spot though. If you 1) allow folder to be set as a default page 2) add new folder to your site root 3) add new page inside new folder and set it folders default page 4) set new folder to site roots default page

-> you will get the folders default page as a site roots default view, but you'll lose 'add new...' and display menus.

However I think this is quite extreme case where you're doing something wrong in the first hand.

I disagree. This is actually a common use case. Example? Say, you want to have your news folder to be the default start page/view of your website. Then you

  1. allow folder to be set as default view (default_page_types under site_properties)
  2. go to myplone.org/select_default_page and select the news folder

Then the not-so-intuitive behaviour begins ;)

  • If you go to myplone.org then, then you do not have an "Add..." menu
  • If you want to add a news item, you first have to click on "View" leading to myplone.org/news/aggregator where you have an "Add news items" menu again and also ending up correctly in the news folder (a very non-intuive way this is)
  • Very puzzling: If you are on myplone.org, then "Contents" leads to /news/folder_contents, whereas "View" leads to /news/aggregator. And /news/ and /news/aggregator then have different "Add..." menus: /news/ lets you add items to the main plone top lever folder (/), whereas /news/aggregator lets add you only news to "news" folder (what you want).

That is, due to this bug, there is no easy way that is free of side effects which allows you to select your news folder as default start page of your website.

I see the point, but I still stand by my opinion that you're doing something very wrong :) What would be the benefit of using a folder (which is using a topic as it's default view) as a default view of a upper level folder? Why not just make a topic to the upper level and use it directly?

I agree thought that this is not ideal solution as people would hit this special condition for sure (as always if you think it is rare :).

comment:29 Changed 19 months ago by eleddy

  • Cc vangheem added
  • Keywords UI added; TuneUp, black-belt, forcommit removed
  • Version changed from 4.1 to 4.3
  • Type changed from Bug to PLIP
  • Milestone changed from 4.3 to 4.4

Ok, I think there is enough history here and in #12199 to warrant that this should be a plip (just like #1299). 1299 is closed so if this gets exciting enough to plip then please change the format, merge the tickets, etc.. Adding vangheem as a cc to see if we can get some UI team review on this still. UI team.....???

comment:30 Changed 19 months ago by encolpe

The easiest solution is to not authorized folderish to be the default item.
If not, the first UI I proposed was using the same than other menus. But at least, with more than twelve items for the PFG it become not really usable.
If we can wait for the PloneConf I will be able to expose usecases we hit in our projects.

comment:31 Changed 17 months ago by vangheem

Sorry about the slow response.

The UI teams position is this: If a folder is selected as the default view of a folder, show a warning message(status message) with a link to the default view's folder_contents view. Then, make sure the folder_contents view's add new menu shows the correct add menu.

Reasons:

  • nothing tricky with add menu
  • showing 2 add menus will be confusing
  • current behavior is unchanged
  • mirrors a lot of other plone behavior: editing default item warning, editing default item portlets warning
  • easy to implement

comment:32 Changed 14 months ago by vangheem

  • Owner changed from pingviini to vangheem

comment:35 Changed 6 months ago by davisagli

  • Milestone changed from 4.4 to 5.0

Milestone 4.4 deleted

Note: See TracTickets for help on using tickets.