|
Description
Pedro Amorim
2023-02-10 12:14:55 UTC
Created attachment 146470 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules In bug 32925 it's discussed the possibility of having a generic fetch function to handle basic CRUD creates and edits for vuejs modules in the future and move all common POST+PUT async requests workflow in there, like the 'Submitting...' loading behaviour. This POC illustrates this and it's intent is to start discussion, plan on iterating further to the rest of ERM if agreed upon. Created attachment 146509 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules In bug 32925 it's discussed the possibility of having a generic fetch function to handle basic CRUD creates and edits for vuejs modules in the future and move all common POST+PUT async requests workflow in there, like the 'Submitting...' loading behaviour. This POC illustrates this and it's intent is to start discussion, plan on iterating further to the rest of ERM if agreed upon. I love it!
Some feedback:
1. a_id => prefer the full var name, agreement_id ;)
2. if ( this.$route.path.includes("edit") )
I don't really like this, I would prefer to rely on the status code (but I do understand why you did that!). Imagine we have another form on this url, it won't work in all cases.
Actually you don't need this, we know already if we are editing or adding (search for "method", if there is agreement_id it's an edit). Note that there is code that is no longer needed there (variable options, etc.)
3. I would add a "count" method in http-client
4. fetch/api-client.js is a too generic name, it should be tight to the "erm" module. Don't you think?
5. Looks like we don't need fetch/fetch.js? Was it expected?
6. There are things in http-client.js we are not using, we should remove them.
7. Instead of the async/await you add, shouldn't we prefer client.agreements.(create).then(addMessage())?
Maybe it's only a matter of taste?
8. Code is not tidy, don't forget to install the pre-commit git hook!
I would like a version of this that is before bug 32925, then we added the "isSubmitting info" there.
Created attachment 146642 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules - Cleaned up the code and further extended the implementation concept on agreements only. - Jonathan I followed your suggestions but had to keep async/await or the catch will not work if an error is thrown from the http-client - I'm keeping this as a separate patch for diff check (intention is to squash all of it once we're happy) - I plan on continuing with this in coming days but more feedback is always welcome, also plan on rebasing this to 32925 Later: - api-client.js should be renamed to erm-api-client.js (or something of the sort) but I kept the same name for now for diff check - erm.js will ultimately disappear once this is all done (or api-client.js will become erm.js) Created attachment 146644 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash Created attachment 146645 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash More updates. Ready for review, for now. Created attachment 146668 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash More updates. Ready for review, for now. JD amended patch: * prettier * remove changes to AgreementRelationships.vue and EHoldingsLocalPackageAgreements.vue to have less files to update in upcoming patches Created attachment 146669 [details] [review] Bug 32939: Have a generic APIClient object You always import the same, only once, and you can access other resources from the same object. Created attachment 146670 [details] [review] Bug 32939: Set the default header at lower level Created attachment 146671 [details] [review] Bug 32939: Restore checkError Created attachment 146672 [details] [review] Bug 32939: Introduce count Not used yet, will replace myFetchTotal Created attachment 146674 [details] [review] Bug 32939: Introduce count Not used yet, will replace myFetchTotal Created attachment 146675 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash More updates. Ready for review, for now. JD amended patch: * prettier * remove changes to AgreementRelationships.vue and EHoldingsLocalPackageAgreements.vue to have less files to update in upcoming patches Created attachment 146676 [details] [review] Bug 32939: Have a generic APIClient object You always import the same, only once, and you can access other resources from the same object. Created attachment 146677 [details] [review] Bug 32939: Set the default header at lower level Created attachment 146678 [details] [review] Bug 32939: Restore checkError Created attachment 146679 [details] [review] Bug 32939: Introduce count Not used yet, will replace myFetchTotal Created attachment 146680 [details] [review] Bug 32939: Fix delete Created attachment 146681 [details] [review] Bug 32939: To be removed Re-add the function we removed earlier, to prevent failures. This patch will be dropped later, when we will touch all the occurrences. Created attachment 146682 [details] [review] Bug 32939: Uniformize api calls We want to make the call easier and without less errors possible. Here, no need to async keyword, try-catch, etc. Only call the method and use the common then with the 2 success and error Promise arguments. We will certainly want to add later a parameter to prevent the display of the error in fetchJSON, in case the failure does not require a message for the end user. Created attachment 147029 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash More updates. Ready for review, for now. JD amended patch: * prettier * remove changes to AgreementRelationships.vue and EHoldingsLocalPackageAgreements.vue to have less files to update in upcoming patches Created attachment 147030 [details] [review] Bug 32939: Have a generic APIClient object You always import the same, only once, and you can access other resources from the same object. Created attachment 147031 [details] [review] Bug 32939: Set the default header at lower level Created attachment 147032 [details] [review] Bug 32939: Restore checkError Created attachment 147033 [details] [review] Bug 32939: Introduce count Not used yet, will replace myFetchTotal Created attachment 147034 [details] [review] Bug 32939: Fix delete Created attachment 147035 [details] [review] Bug 32939: Uniformize api calls We want to make the call easier and without less errors possible. Here, no need to async keyword, try-catch, etc. Only call the method and use the common then with the 2 success and error Promise arguments. We will certainly want to add later a parameter to prevent the display of the error in fetchJSON, in case the failure does not require a message for the end user. Created attachment 147036 [details] [review] Bug 32939: Use APIClient to fetch agreements Created attachment 147037 [details] [review] Bug 32939: Use APIClient to fetch licenses Created attachment 147038 [details] [review] Bug 32939: Use APIClient to fetch patrons Created attachment 147039 [details] [review] Bug 32939: Use APIClient to fetch vendors Created attachment 147040 [details] [review] Bug 32939: Use APIClient to fetch local eHoldings Created attachment 147041 [details] [review] Bug 32939: Use APIClient to fetch EBSCO eHoldings Created attachment 147042 [details] [review] Bug 32939: Remove fetch/erm.js Created attachment 147043 [details] [review] Bug 32939: Use APIClient to replace some more fetch calls Created attachment 147044 [details] [review] Bug 32939: Use APIClient to replace PATCH requests Test plan: Retest the whole ERM module. Needs 32981 applied - added to dependencies Created attachment 147056 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash More updates. Ready for review, for now. JD amended patch: * prettier * remove changes to AgreementRelationships.vue and EHoldingsLocalPackageAgreements.vue to have less files to update in upcoming patches Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147057 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash More updates. Ready for review, for now. JD amended patch: * prettier * remove changes to AgreementRelationships.vue and EHoldingsLocalPackageAgreements.vue to have less files to update in upcoming patches Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147059 [details] [review] Bug 32939: Have a generic APIClient object You always import the same, only once, and you can access other resources from the same object. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147060 [details] [review] Bug 32939: Set the default header at lower level Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147061 [details] [review] Bug 32939: Restore checkError Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147062 [details] [review] Bug 32939: Introduce count Not used yet, will replace myFetchTotal Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147063 [details] [review] Bug 32939: Fix delete Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147064 [details] [review] Bug 32939: Uniformize api calls We want to make the call easier and without less errors possible. Here, no need to async keyword, try-catch, etc. Only call the method and use the common then with the 2 success and error Promise arguments. We will certainly want to add later a parameter to prevent the display of the error in fetchJSON, in case the failure does not require a message for the end user. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147065 [details] [review] Bug 32939: Use APIClient to fetch agreements Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147066 [details] [review] Bug 32939: Use APIClient to fetch licenses Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147067 [details] [review] Bug 32939: Use APIClient to fetch patrons Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147068 [details] [review] Bug 32939: Use APIClient to fetch vendors Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147069 [details] [review] Bug 32939: Use APIClient to fetch local eHoldings Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147070 [details] [review] Bug 32939: Use APIClient to fetch EBSCO eHoldings Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147071 [details] [review] Bug 32939: Remove fetch/erm.js Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147072 [details] [review] Bug 32939: Use APIClient to replace some more fetch calls Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 147073 [details] [review] Bug 32939: Use APIClient to replace PATCH requests Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> I wonder if it would be useful to implement a simple feature or a Koha plugin using Vue.js to see which components need to be specific to ERM and which need to be more generalized/re-usable... (In reply to David Cook from comment #56) > I wonder if it would be useful to implement a simple feature or a Koha > plugin using Vue.js to see which components need to be specific to ERM and > which need to be more generalized/re-usable... I am already working on another module, on top of this tree. This is the result of the different situations I faced. (In reply to Jonathan Druart from comment #57) > (In reply to David Cook from comment #56) > > I wonder if it would be useful to implement a simple feature or a Koha > > plugin using Vue.js to see which components need to be specific to ERM and > > which need to be more generalized/re-usable... > > I am already working on another module, on top of this tree. This is the > result of the different situations I faced. Great! I'm very busy the next week, but sometime soon I want to stay online late, so that we can chat more about Vue.js in Koha on IRC. Created attachment 147306 [details] [review] Bug 32939: Have a generic fetch function for POST and PUT requests in vue modules Rebase to 32806 and prior to 32925. Squash More updates. Ready for review, for now. JD amended patch: * prettier * remove changes to AgreementRelationships.vue and EHoldingsLocalPackageAgreements.vue to have less files to update in upcoming patches Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147307 [details] [review] Bug 32939: Have a generic APIClient object You always import the same, only once, and you can access other resources from the same object. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147308 [details] [review] Bug 32939: Set the default header at lower level Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147309 [details] [review] Bug 32939: Restore checkError Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147310 [details] [review] Bug 32939: Introduce count Not used yet, will replace myFetchTotal Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147311 [details] [review] Bug 32939: Fix delete Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147312 [details] [review] Bug 32939: Uniformize api calls We want to make the call easier and without less errors possible. Here, no need to async keyword, try-catch, etc. Only call the method and use the common then with the 2 success and error Promise arguments. We will certainly want to add later a parameter to prevent the display of the error in fetchJSON, in case the failure does not require a message for the end user. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147313 [details] [review] Bug 32939: Use APIClient to fetch agreements Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147314 [details] [review] Bug 32939: Use APIClient to fetch licenses Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147315 [details] [review] Bug 32939: Use APIClient to fetch patrons Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147316 [details] [review] Bug 32939: Use APIClient to fetch vendors Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147317 [details] [review] Bug 32939: Use APIClient to fetch local eHoldings Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147318 [details] [review] Bug 32939: Use APIClient to fetch EBSCO eHoldings Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147319 [details] [review] Bug 32939: Remove fetch/erm.js Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147320 [details] [review] Bug 32939: Use APIClient to replace some more fetch calls Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147321 [details] [review] Bug 32939: Use APIClient to replace PATCH requests Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 147322 [details] [review] Bug 32939: (follow-up) Use APIClient to fetch vendors Really cool to see how this thing is evolving! Created attachment 147332 [details] [review] Bug 32939: (follow-up) Use APIClient to fetch vendors Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Pushed to master for 23.05. Nice work everyone, thanks! Nice work everyone! Pushed to stable for 22.11.x Not needed in 22.05.x Created attachment 149441 [details] [review] Bug 32939: Fix title endpoint (In reply to Jonathan Druart from comment #81) > Created attachment 149441 [details] [review] [review] > Bug 32939: Fix title endpoint Moved to bug 33485. |