-
Notifications
You must be signed in to change notification settings - Fork 53
pkp/pkp-lib#9295 continuous publication implementation update #683
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
Conversation
59b21c7
to
b3f6fcf
Compare
6966b39
to
0f61ced
Compare
@jardakotesovec, sending to you for review. |
@asmecher I would prefer not to merge this version, and still make the adjustments to use existing form fields, and not creating new field which combines multiple fields. Example I provided is available on these PRs.. |
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.
Hi Touhidur,
thanks for the updates, I provided some feedback that should help to clean this up. Feel free to refer to my original example, which was done quite carefully and was done with good intentions to illustrate good patterns to fully take advantage of our composables and composition api.
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
47b92a6
to
61effaf
Compare
async (newForm) => { | ||
if (newForm && props.formName === 'issue' && isOJS()) { | ||
await useWorkflowPublicationFormIssue(newForm); | ||
} |
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.
In general custom logic for forms has been solved by creating dedicated component for that form.. But we can handle that once we would maybe migrate the whole form to client side.. Until than applying the custom logic here I think is fine.
Just to follow composable conventions which we use, is that the composable returns what might be useful - refs, computed properties and methods.
In this case you can return the initialise
and call it when necessary.
const {initialize} = useWorkflowPublicationFormIssue(newForm)
initialize();
That improves the convention but also gives you flexibility when you want to do initial fetching.
I see that you decided to keep also the same logic server side, which is fine its bit of duplication but it help avoiding cascading request (fetching form first, and than trigering additional fetches) so thats good.
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.
decided to keep also the same logic server side, which is fine its bit of duplication
if we want to remove it, that would be very easy as well . just remove the server side code that build the fields and in the composable useWorkflowPublicationFormIssue
remove the check of field existence as other parts are just same . Only one thing we need to figure out in that case is how to position the fields before/after certain field using the useForm
.
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.
I would be supportive of this.
To resolve the positioning - solution that we want to add to the useForm at some point anyway, is to introduce the same options as we have in FormComponent.php for $position.
It would go as new option, which is 3rd parameter to the addField* functions. We can address that separately as out of scope of this. Or add that option as part of the PR - both options are fine with me.
example would be
addFieldText('fieldName', {inputType: 'text',...}, {positionAfter: 'anotherFieldName'}
addFieldText('fieldName', {inputType: 'text',...}, {positionBefore: 'anotherFieldName'}
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
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.
Hi Touhidur,
Its lot of changes, I tried to get through everything in detail.. let me know if something is unclear.
b88a18c
to
7237121
Compare
src/pages/workflow/composables/useWorkflowPublicationFormIssue.js
Outdated
Show resolved
Hide resolved
…status as STATUS_READY_TO_PUBLISH
…submission and publication) for status const
d61e83f
to
271cca6
Compare
@@ -1,3 +1,4 @@ | |||
<!-- TODO: need this anymore ? remove this after testing --> |
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.
Any thoughts on this? I don't see FieldSelectIssue used anywhere.. so I am supportive to remove it.. and just make sure that the tests are passing.
/** | ||
* Check if the required fields already exist in the form | ||
*/ | ||
async function checkFieldsExist() { |
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.
Just small clean up - this does not need to be async function, right?
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.
Just two clean up suggestions. I think its ready for merging.
Thanks!
for pkp/pkp-lib#9295 , second batch of PRs to update the implementation to
published
column inpublications
tablestatus
column ofpublications
table by introducing a new statusPKPSubmission::STATUS_READY_TO_PUBLISH