apostrophecms / apostrophe

A full-featured, open-source content management framework built with Node.js that empowers organizations by combining in-context editing and headless architecture in a full-stack JS environment.
https://apostrophecms.com
Other
4.36k stars 590 forks source link

3.x: custom piece admin filter of type select breaks admin UI #2916

Open myovchev opened 3 years ago

myovchev commented 3 years ago

To Reproduce

  1. Create a piece module having field category of type select and add filter:
    module.exports = {
    // ...
    filters: {
    add: {
      category: {
        label: 'Category',
        type: 'select',
        choices: [
          {
            label: 'Cat 1',
            value: 'cat1'
          },
          {
            label: 'Cat 2',
            value: 'cat2'
          }
        ]
      }
    }
    }
    // ...
    };
  2. Add new record
  3. After submission, when you come back to the list, it's empty
  4. The API route called is: api/v1/tags?apos-mode=draft&archived=false&category&page=1 (note the category query string)
  5. The list will be empty until you use the filter and choose a value. You are not able to see all results.

Fix attempt 1

  1. change the filter to have default value null
    module.exports = {
    // ...
    filters: {
    add: {
      category: {
        label: 'Category',
        type: 'select',
        choices: [
          {
            label: 'Cat 1',
            value: 'cat1'
          },
          {
            label: 'Cat 2',
            value: 'cat2'
          }
        ],
        def: null
      }
    }
    }
    };
  2. same result as before with same API route called

Fix attempt 2 (different behavior)

  1. add manually a null value choice, don't use default value
    module.exports = {
    // ...
    filters: {
    add: {
      category: {
        label: 'Category',
        type: 'select',
        choices: [
          {
            label: 'Any',
            value: null
          },
          {
            label: 'Cat 1',
            value: 'cat1'
          },
          {
            label: 'Cat 2',
            value: 'cat2'
          }
        ]
      }
    }
    }
    };
  2. the list is now visible
  3. filter by any non null category value (works as expected)
  4. try to edit a record - record not found notification
  5. change to Any - empty list again

In all of the cases above (list) the corresponding query log is:

{
  '$and': [
    { archived: { '$ne': true } },
    { type: 'tags' },
    { '$or': [ { aposLocale: 'en:draft' }, { aposLocale: null } ] },
    { category: { '$in': [ '' ] } }
  ]
}

Expected behavior

To work as expected. However they are different layers of this bug. It's not entirely clear to me if the UI don't have to send null values in this case, or the backend should properly filter them out. I can see a need of backend to build somehow in some cases a field=null db criteria...

Additionally, I think the edit (getOne) API request made from admin UI should ignore the filters explicitly.

Describe the bug

The filter is breaking the API requests for list and edit. Details in the reproduce section

Details

Version of Node.js: v12.21.0

Operating System: Ubuntu 20.04.2 LTS

boutell commented 3 years ago

We haven't forgotten about this issue! Been working hard to ship beta 1 today (: Thanks for your patience.

My first impression is that in general the filters configuration needs to be better able to leverage what is already known about the corresponding schema field. Currently it seems a bit redundant.

On Fri, Apr 16, 2021 at 1:46 PM Miro Yovchev @.***> wrote:

To Reproduce

  1. Create a piece module having field category of type select and add filter:

module.exports = {// ...filters: { add: { category: { label: 'Category', type: 'select', choices: [ { label: 'Cat 1', value: 'cat1' }, { label: 'Cat 2', value: 'cat2' } ] } } }// ...};

  1. Add new record
  2. After submission, when you come back to the list, it's empty
  3. The API route called is: api/v1/tags?apos-mode=draft&archived=false&category&page=1 (note the category query string)
  4. The list will be empty until you use the filter and choose a value. You are not able to see all results.

Fix attempt 1

  1. change the filter to have default value null

module.exports = { // ... filters: { add: { category: { label: 'Category', type: 'select', choices: [ { label: 'Cat 1', value: 'cat1' }, { label: 'Cat 2', value: 'cat2' } ], def: null } } }};

  1. same result as before with same API route called

Fix attempt 2 (different behavior)

  1. add manually a null value choice, don't use default value

module.exports = { // ... filters: { add: { category: { label: 'Category', type: 'select', choices: [ { label: 'Any', value: null }, { label: 'Cat 1', value: 'cat1' }, { label: 'Cat 2', value: 'cat2' } ] } } }};

  1. the list is now visible
  2. filter by any non null category value (works as expected)
  3. try to edit a record - record not found notification
  4. change to Any - empty list again

In all of the cases above (list) the corresponding query log is:

{ '$and': [ { archived: { '$ne': true } }, { type: 'tags' }, { '$or': [ { aposLocale: 'en:draft' }, { aposLocale: null } ] }, { category: { '$in': [ '' ] } } ]}

Expected behavior

To work as expected. However they are different layers of this bug. It's not entirely clear to me if the UI don't have to send null values in this case, or the backend should properly filter them out. I can see a need of backend to build somehow in some cases a field=null db criteria...

Additionally, I think edit the record API request made from admin UI should ignore the filters explicitly. Describe the bug

The filter is breaking the API requests for list and edit. Details in the reproduce section Details

Version of Node.js: v12.21.0

Operating System: Ubuntu 20.04.2 LTS

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/2916, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27PE5UBNAJJVRYFLMM3TJBZXFANCNFSM43B6ODEQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

myovchev commented 3 years ago

Completely agree about the redundancy. If the filter provided is part of the schema, there is no need re-enter the details (except when in a need of override). This issue is in no way a high priority and I suspect it needs clear head and no rush to be solved properly.