Feat: Allow writeable & nonvisible fields as default sort #17205
Conversation
Marc-Roig
added
source: core:content-manager
Marc-Roig
requested review from
gu-stav,
alexandrebodin,
Feranchz,
markkaylor and
jhoward1994
Marc-Roig
removed
the
flag: don't merge
|
@alexandrebodin done! 🚀 |
| * 3. Filters by updated_by (successfully) | ||
| */ | ||
|
|
||
| describe('Test type decimal', () => { |
There was a problem hiding this comment.
oops yes ! On my way!
|
I will schedule this for the 4.12.0 release |
Hey to avoid having to maintain milestone I recommend to assign PRs to milestones at merge time when it is possible :) |
| defaultSortBy: yup.string().oneOf(validAttributes.concat('id')).default('id'), | ||
| defaultSortBy: yup | ||
| .string() | ||
| .oneOf(validAttributes.concat(['id', ...nonVisibleWritableAttributes])) |
There was a problem hiding this comment.
I was wondering should we allow sorting by created by & updated by also (maybe the other PR should do that 🤔 )
There was a problem hiding this comment.
updated to allow also creator fields f6543f7 (#17205)
| @@ -22,6 +26,13 @@ module.exports = (schema, opts = {}) => | |||
| const createSettingsSchema = (schema) => { | |||
| const validAttributes = Object.keys(schema.attributes).filter((key) => isListable(schema, key)); | |||
|
|
|||
| // TODO V5: Refactor non visible fields to be a part of content-manager schema | |||
| const model = strapi.getModel(schema.uid); | |||
| const nonVisibleWritableAttributes = intersection( | |||
There was a problem hiding this comment.
Just noticed localizations end up in that list 🤔 I'm not sure that makes sense but not sure we can set them as non writable. Do you mind checking that ?
There was a problem hiding this comment.
👍 I will check it. Do you think it's a problem if we allow to sort by localizations & locale?
There was a problem hiding this comment.
Sorting by those fields is already allowed as of today. Not in the UI, but the api allows you to do so.
There was a problem hiding this comment.
Hum 🤔 by locale yes localizations I'm surprised but if that's the case then there is no reason you can do it in the API and not the Admin even if that would make much sense to filter by localizations anyway. We will remove that in v5
There was a problem hiding this comment.
Agree. I only have seen some people filtering entries which their localization have some value set.
I can not think of usecases where you would want to sort by localizations in the admin api 🤔
Still, I will take a look if it's safe to move localizations to be nonwritable
There was a problem hiding this comment.
Tested the API endpoints and could test filtering on multiple different fields including strapi_reviewWorkflows_stage correctly.
The only thing I'm not a fan with merge this now is that those fields don't appear in the UI so I get empty inputs in the configure the view if I defined them in the defaultSortBy. Should we merge this with some frontend ?
|
@alexandrebodin what do you think would be the appropiate change here :
cc @gu-stav |
I would say we should ensure both lists (FE/BE) are the same (displayed & validated against) |
|
@alexandrebodin I think that could be tricky today, because forntend displays all the visible fields that the backend returns. Our initial hope was to have another definition of visible fields, but turned out that will not be possible until v5. Do you think we should push for ensuring this lists are consistent at this moment? I have the feeling is quite a big amount of work when we will also be refactoring it on v5 soon. |
Do we have a choice ? we will need to give the right info to the frontend if we want to allow filtering on those attributes they need to appear in the UI right? unless you just need it for the API and in that case I would prevent being able to set the as defaultSortBy probably :) For how we could do it; I would probably go with an other approach knowing we can refactor it in v5. Something like adding this list of fields as a new info in the Schema the API returns. This would avoid all the side effects I think 🤔 |
|
@alexandrebodin I think at the moment is a bit tricky, because frontend doesn't yet filter by relations. And it's something we will do in the RW work. Our original idea was to add those fields in the frontend (until we work on this on v5), handle relational sorting & filtering and showing the strapi stage field in the List View table like we show now.
Just in case we would want to return those fields in the api, I imagine we would be returning that in the /configuration endpoint. Does it really solve the issue? I think it would extend more in time the problem and adding more complexity in code that will have to be removed in months. My guess is, for a content type settings you propose: {
"uid": "api::category.category",
"settings": {
"bulkable": true,
"filterable": true,
"searchable": true,
"pageSize": 10,
"mainField": "name",
"defaultSortBy": "name",
"defaultSortOrder": "ASC"
},
"metadatas": {
...
"strapi_stage": {
"edit": {
"label": "name",
"description": "",
"placeholder": "",
"visible": false,
"editable": true
},
"list": {
"label": "name",
"searchable": true,
"sortable": true
"visible": true // <= make it visible here?
}
},
}
} |
gu-stav
force-pushed
the
feature/rw-stage-default-sort
branch
3 times, most recently
from
c408f28
to
ec91ecb
Compare
gu-stav
force-pushed
the
feature/rw-stage-default-sort
branch
from
ec91ecb
to
d112d1a
Compare
gu-stav
force-pushed
the
feature/rw-stage-default-sort
branch
from
d112d1a
to
78f048b
Compare
|
Size Change: -7 B (0%) Total Size: 1.53 MB ℹ️ View Unchanged
|
gu-stav
force-pushed
the
feature/rw-stage-default-sort
branch
2 times, most recently
from
c780fe3
to
be6b0ad
Compare
…nto default-sort-by-non-visible-attrs/resolve-merge
…rs/resolve-merge
Marc-Roig
merged commit 47e1cc1
into
feature/rw-stage-default-sort
|
@Marc-Roig I think the method is called from here: Do you want I create an issue? |
|
Hey @vbornand thank you! I will take a look tomorrow |
What does it do?
This work allows filtering and sorting by fields that are not visible but writtable in the Admin API, some example of such fields:
We have the requirement to filter and sort by those fields. Because of that it makes sense to allow setting those fields as a default sort also.
This is a workaround to make it work in V4, but we will be refactoring this work in V5. This PR allows those fields to be filtered and sorted, and so we will need to hardcode those fields in the frontend side.