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
Allow multiple and other authors on articles #3566
Conversation
e386a97
to
7eb5085
Compare
7eb5085
to
b32b3c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
You should rename author
to authors
to reflect this change. And fix up the small other things and this is good to go :)
Regarding removing the created_by, I think it might be a nice thing to keep?
</span> | ||
</div> | ||
|
||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still here 😬
b32b3c8
to
b962f02
Compare
On default the author field will be the current user, and if the field is empty it will also be set to the current user upon validation as a security measure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for suddenly finding more stuff here, I should have done a more thorough review the first time around 😞.
A couple things (as are mentioned in the comments):
- If authors is a required field in the backend, it should always be defined here, and both the types, and the usage of the
authors
field should reflect that. So no null checks or anything here (array length checks are fine). Let the type system catch it. - Right now, the
authors
field is set together with theObjectPermissions
. This is not really related, and ObjectPermissions are generic fields that can be used on any field that use objectPermissions. I do like the idea of having default fields that can be reused super easy, but I doubt that we would have authors anywhere else than articles so with that in mind, both that it's not generic and that it does not have anything to do with ObjectPermissions, I think you should move it out and put the code for authors in the articleeditor directly.
</span> | ||
</div> | ||
|
||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still here 😬
b962f02
to
a74f318
Compare
8398ae0
to
50b66bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍 looks good now. Up to you if you want to fix up the last issues
component={SelectInput.AutocompleteField} | ||
isMulti | ||
filter={['users.user']} | ||
id="article-title" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm
type ValidationError<T> = Partial<{ | ||
[key in keyof T]: string | Record<string, string>[]; | ||
}>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loving the types😍
if (!data.authors || data.authors.length === 0) { | ||
errors.title = 'Forfatter er påkrevd'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also be able to create a custom validation function for this and use createValidator
like before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is entirely fine imo. though.
50b66bb
to
8cc4b97
Compare
Any reason to not merge this in now? @jonasdeluna You've done great work! |
Thanks, not really unless the validator needs changing ill have a final look at the test today/in an hour |
d22eece
to
11da5b0
Compare
Everything should be good, but messed up the rebase :| also the backend tests are failing... |
11da5b0
to
0836303
Compare
9fa417d
to
5c959fa
Compare
…rs and refactor some code
66445b5
to
2ea8faf
Compare
Description
Option to add multiple/other users as author on articles.
Result
Before:
After:
Before:
After:
After:
Testing
Depends on: webkom/lego#3220
Resolves: webkom/lego#3195