Contributing batch requisition endpoints to core

Hello tech committee!

  During the next couple of days, the Malawi team is planning to work on contributing the batch requisition endpoints from our service () to the core requisition service. We would therefore like to confirm technical details for this change and a necessary refactor of the error handling in requisitions.

  The RAML has already been reviewed on May, during the first attempt to contribute those, although some things have changed since then, so I have uploaded it for review again. Please do take a look: Specifically:  - minified DTOs are used in all of those endpoints and they contain only the data necessary for the batch approval page. Working with several requisitions, each having a few hundred products made requests and responses really huge and so it proved necessary to satisfy slow connections  - a new endpoint has been introduced to also retrieve requisitions in batch (next to save & approve). It takes an array of UUIDs and will ignore UUIDs that do not exist and UUIDs of requisitions the user does not have access to  - partial success (minimum 1 requisition processed with validation errors) returns error code 400

  Since the batch endpoints need to always process all passed requisitions, a refactor of error handling is necessary in the requisition service. Currently, most validation checks, including permission verification, rely on unchecked exceptions. Throwing an unchecked exception, however, ceases the execution and would therefore lead to the batch endpoints not processing all passed requisitions upon a validation error. Catching unchecked exception was not accepted as viable option by the technical committee and a refactor of error handling was suggested.

  As a part of this contribution we are therefore planning to re-work the validation checks in the requisition service, such that they do no longer immediately answer with an exception, but rather return an instance of "ValidationResult" class. This new class would store an array of error Message objects and would contain some utility methods to retrieve the errors and to check if the validation passed. It will be up to an endpoint then what to do with that error. Current requisition endpoints will translate the validation messages to exceptions, while the new batch endpoints will store the errors and keep on processing all requisitions.

  Does all of this sound/look good to you? If you have any further questions or concerns, please let me know as well.

Best regards,

  Sebastian.
···

https://github.com/OpenLMIS-Malawi/mw-requisition-expansion
https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c


Sebastian Brudziński

    Software Developer / Team Leader

sbrudzinski@soldevelo.com

SolDevelo
Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

Hi Sebastian,

At a high level the work on validations, not immediately throwing an unchecked exception, etc all sound familiar and on the right course.

If I’m reading this RAML right, these two statements are true:

  1. Batch saving doesn’t take in the request a minified Requisition
  2. Batch approve takes in a set of UUIDs (POSTed) and returns a minifed Requisition
    Is that right?

I spent a few minutes tracking down some of these older conversations, so I’ll link them here for reference:

···

On Wednesday, July 19, 2017 at 8:10:56 AM UTC-7, Sebastian Brudziński wrote:

Hello tech committee!

  During the next couple of days, the Malawi team is planning to work on contributing the batch requisition endpoints from our service ([https://github.com/OpenLMIS-Malawi/mw-requisition-expansion](https://github.com/OpenLMIS-Malawi/mw-requisition-expansion)      ) to the core requisition service. We would therefore like to confirm technical details for this change and a necessary refactor of the error handling in requisitions.
  The RAML has already been reviewed on May, during the first attempt to contribute those, although some things have changed since then, so I have uploaded it for review again. Please do take a look: [https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c](https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c)

  Specifically:

   - minified DTOs are used in all of those endpoints and they contain only the data necessary for the batch approval page. Working with several requisitions, each having a few hundred products made requests and responses really huge and so it proved necessary to satisfy slow connections

   - a new endpoint has been introduced to also retrieve requisitions in batch (next to save & approve). It takes an array of UUIDs and will ignore UUIDs that do not exist and UUIDs of requisitions the user does not have access to

   - partial success (minimum 1 requisition processed with validation errors) returns error code 400
  Since the batch endpoints need to always process all passed requisitions, a refactor of error handling is necessary in the requisition service. Currently, most validation checks, including permission verification, rely on unchecked exceptions. Throwing an unchecked exception, however, ceases the execution and would therefore lead to the batch endpoints not processing all passed requisitions upon a validation error. Catching unchecked exception was not accepted as viable option by the technical committee and a refactor of error handling was suggested.
  As a part of this contribution we are therefore planning to re-work the validation checks in the requisition service, such that they do no longer immediately answer with an exception, but rather return an instance of "ValidationResult" class. This new class would store an array of error Message objects and would contain some utility methods to retrieve the errors and to check if the validation passed. It will be up to an endpoint then what to do with that error. Current requisition endpoints will translate the validation messages to exceptions, while the new batch endpoints will store the errors and keep on processing all requisitions.
  Does all of this sound/look good to you? If you have any further questions or concerns, please let me know as well.

Best regards,

  Sebastian.


Sebastian Brudziński

    Software Developer / Team Leader


     sbrudzinski@soldevelo.com


SolDevelo
Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

Hi Josh.

For the first point - this is actually a mistake in RAML. All of those enpoints operate on minified requisitions or UUIDs.

For the second point - that is correct.

Regards,

Sebastian.


SolDevelo
Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

···

On Fri, Jul 21, 2017 at 10:14 PM, Josh Zamor josh.zamor@villagereach.org wrote:

Hi Sebastian,

At a high level the work on validations, not immediately throwing an unchecked exception, etc all sound familiar and on the right course.

If I’m reading this RAML right, these two statements are true:

  1. Batch saving doesn’t take in the request a minified Requisition
  2. Batch approve takes in a set of UUIDs (POSTed) and returns a minifed Requisition
    Is that right?

I spent a few minutes tracking down some of these older conversations, so I’ll link them here for reference:

On Wednesday, July 19, 2017 at 8:10:56 AM UTC-7, Sebastian Brudziński wrote:

Hello tech committee!

  During the next couple of days, the Malawi team is planning to work on contributing the batch requisition endpoints from our service ([https://github.com/OpenLMIS-Malawi/mw-requisition-expansion](https://github.com/OpenLMIS-Malawi/mw-requisition-expansion)      ) to the core requisition service. We would therefore like to confirm technical details for this change and a necessary refactor of the error handling in requisitions.
  The RAML has already been reviewed on May, during the first attempt to contribute those, although some things have changed since then, so I have uploaded it for review again. Please do take a look: [https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c](https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c)

  Specifically:

   - minified DTOs are used in all of those endpoints and they contain only the data necessary for the batch approval page. Working with several requisitions, each having a few hundred products made requests and responses really huge and so it proved necessary to satisfy slow connections

   - a new endpoint has been introduced to also retrieve requisitions in batch (next to save & approve). It takes an array of UUIDs and will ignore UUIDs that do not exist and UUIDs of requisitions the user does not have access to

   - partial success (minimum 1 requisition processed with validation errors) returns error code 400
  Since the batch endpoints need to always process all passed requisitions, a refactor of error handling is necessary in the requisition service. Currently, most validation checks, including permission verification, rely on unchecked exceptions. Throwing an unchecked exception, however, ceases the execution and would therefore lead to the batch endpoints not processing all passed requisitions upon a validation error. Catching unchecked exception was not accepted as viable option by the technical committee and a refactor of error handling was suggested.
  As a part of this contribution we are therefore planning to re-work the validation checks in the requisition service, such that they do no longer immediately answer with an exception, but rather return an instance of "ValidationResult" class. This new class would store an array of error Message objects and would contain some utility methods to retrieve the errors and to check if the validation passed. It will be up to an endpoint then what to do with that error. Current requisition endpoints will translate the validation messages to exceptions, while the new batch endpoints will store the errors and keep on processing all requisitions.
  Does all of this sound/look good to you? If you have any further questions or concerns, please let me know as well.

Best regards,

  Sebastian.


Sebastian Brudziński

    Software Developer / Team Leader


     sbrudzinski@soldevelo.com


SolDevelo
Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

You received this message because you are subscribed to the Google Groups “OpenLMIS Dev” group.

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

To post to this group, send email to openlmis-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/34d2dc9d-0372-430b-8bbf-462389afac3f%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

Thanks Sebastian,

Lets update the RAML, that seems like a simple step.

I think everything here works, though I’d like us to take a step back and look at these endpoints from the REST perspective. Overall I feel like we’ve been slipping in building a RESTful interface (especially when it comes to performance hacks and finding things), and when I look at the endpoints the various services provide, I’m growing concerned.

For Batch Requisitions, The RESTful representation for /requisitions/batch/save, /requisitions/batch/approve, etc just doesn’t feel very RESTful. They look more like method calls. More-over when we say “batch” here, we seem to mean “batch approval” - not just batch. Overall I’m wondering if creating a new approval resource would help keep the boundary between “requisition” and “approval” clearer. After all the two modes are quite distinct: I can’t change approval fields when it’s not in the approval phase, nor can I change requisition fields when it is in the approval phase. Sure the screen needs to display most of these fields, but that doesn’t mean that it’s the same RESTful resource. Longer term could you see us splitting out approval as a separate (sub)resource?

There is an immediate improvement that could be made to these proposed endpoints, before that’s done, based on this stack overflow answer:

  • I don’t see any reason for POST /api/requisitions/batch/approve to exist. In fact it highlights a problem with our current approach which looks more like method calls. We currently have POST /api/requisitions/{id}/approve, and /api/requisitions/{id}/authorize, and /api/requisitions/{id}/reject, etc. If we had instead supported POST /api/requisitions?markAsApproved&id=A&id=B&id=… (or have ids in POST body) it would have been easy to support the batch approval feature (it would already exist) and we’d have fewer verbs in our URI (meaning fewer endpoints that look like method calls).
    I think it’d be worth a bit of inconsistency in the short term if we could adopt this for the batch approval need, rather than introduce another approve endpoint (/api/requisitions/batch/approve).
···

On Friday, July 21, 2017 at 1:27:20 PM UTC-7, Sebastian Brudziński wrote:

Hi Josh.

For the first point - this is actually a mistake in RAML. All of those enpoints operate on minified requisitions or UUIDs.

For the second point - that is correct.

Regards,

Sebastian.

On Fri, Jul 21, 2017 at 10:14 PM, Josh Zamor josh.zamor@villagereach.org wrote:

Hi Sebastian,

At a high level the work on validations, not immediately throwing an unchecked exception, etc all sound familiar and on the right course.

If I’m reading this RAML right, these two statements are true:

  1. Batch saving doesn’t take in the request a minified Requisition
  2. Batch approve takes in a set of UUIDs (POSTed) and returns a minifed Requisition
    Is that right?

I spent a few minutes tracking down some of these older conversations, so I’ll link them here for reference:

On Wednesday, July 19, 2017 at 8:10:56 AM UTC-7, Sebastian Brudziński wrote:

Hello tech committee!

  During the next couple of days, the Malawi team is planning to work on contributing the batch requisition endpoints from our service ([https://github.com/OpenLMIS-Malawi/mw-requisition-expansion](https://github.com/OpenLMIS-Malawi/mw-requisition-expansion)      ) to the core requisition service. We would therefore like to confirm technical details for this change and a necessary refactor of the error handling in requisitions.
  The RAML has already been reviewed on May, during the first attempt to contribute those, although some things have changed since then, so I have uploaded it for review again. Please do take a look: [https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c](https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c)

  Specifically:

   - minified DTOs are used in all of those endpoints and they contain only the data necessary for the batch approval page. Working with several requisitions, each having a few hundred products made requests and responses really huge and so it proved necessary to satisfy slow connections

   - a new endpoint has been introduced to also retrieve requisitions in batch (next to save & approve). It takes an array of UUIDs and will ignore UUIDs that do not exist and UUIDs of requisitions the user does not have access to

   - partial success (minimum 1 requisition processed with validation errors) returns error code 400
  Since the batch endpoints need to always process all passed requisitions, a refactor of error handling is necessary in the requisition service. Currently, most validation checks, including permission verification, rely on unchecked exceptions. Throwing an unchecked exception, however, ceases the execution and would therefore lead to the batch endpoints not processing all passed requisitions upon a validation error. Catching unchecked exception was not accepted as viable option by the technical committee and a refactor of error handling was suggested.
  As a part of this contribution we are therefore planning to re-work the validation checks in the requisition service, such that they do no longer immediately answer with an exception, but rather return an instance of "ValidationResult" class. This new class would store an array of error Message objects and would contain some utility methods to retrieve the errors and to check if the validation passed. It will be up to an endpoint then what to do with that error. Current requisition endpoints will translate the validation messages to exceptions, while the new batch endpoints will store the errors and keep on processing all requisitions.
  Does all of this sound/look good to you? If you have any further questions or concerns, please let me know as well.

Best regards,

  Sebastian.


Sebastian Brudziński

    Software Developer / Team Leader


     sbrudzinski@soldevelo.com


SolDevelo
Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

You received this message because you are subscribed to the Google Groups “OpenLMIS Dev” group.

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

To post to this group, send email to openlmis-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/34d2dc9d-0372-430b-8bbf-462389afac3f%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.


SolDevelo
Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

Yes, I think treating the requisition ready for approval as a separate (sub)resource makes sense from the system’s perspective. It could also simplify the validation checks that we currently have in place and that make sure that only specific fields are changed during approval. When treating them as separate resources we could just remove all those checks, as no other fields than those that can be edited, would be passed and accepted by the endpoint.

  As for your proposed endpoint design - would you like to follow the same approach for the remaining "batch approval" endpoints for now? Like "/api/requisitions?retrieve&id=1&id=2...". I don't have a problem with that, although I think it looks a little more messy than the originally proposed design (even if it wasn't very RESTful). I can go ahead and code them either way though.

Regards,

  Sebastian.
···

On 24.07.2017 20:29, Josh Zamor wrote:

Thanks Sebastian,

    Lets update the RAML, that seems like a simple step. 



    I think everything here works, though I'd like us to take a step back and look at these endpoints from the REST perspective.  Overall I feel like we've been slipping in building a RESTful interface (especially when it comes to performance hacks and finding things), and when I look at the endpoints the various services provide, I'm growing concerned. 



    For Batch Requisitions, The RESTful representation for /requisitions/batch/save, /requisitions/batch/approve, etc just doesn't feel very RESTful.  They look more like method calls.  More-over when we say "batch" here, we seem to mean "batch approval" - not just batch.  Overall I'm wondering if creating a new approval resource would help keep the boundary between "requisition" and "approval" clearer.  After all the two modes are quite distinct: I can't change approval fields when it's not in the approval phase, nor can I change requisition fields when it is in the approval phase.  Sure the screen needs to display most of these fields, but that doesn't mean that it's the same RESTful resource.  Longer term could you see us splitting out approval as a separate (sub)resource?



    There is an immediate improvement that could be made to these proposed endpoints, before that's done, based on [this](https://stackoverflow.com/questions/511281/patterns-for-handling-batch-operations-in-rest-web-services)
    stack overflow answer:
  •         I don't see any reason for POST /api/requisitions/batch/approve to exist.  In fact it highlights a problem with our current approach which looks more like method calls.  We currently have POST /api/requisitions/{id}/approve, and /api/requisitions/{id}/authorize, and /api/requisitions/{id}/reject, etc.  If we had instead supported POST /api/requisitions?markAsApproved&id=A&id=B&id=... (or have ids in POST body) it would have been *easy*
            to support the batch approval feature (it would already exist) and we'd have fewer verbs in our URI (meaning fewer endpoints that look like method calls).
        I think it'd be worth a bit of inconsistency in the short term if we could adopt this for the batch approval need, rather than introduce another approve endpoint (/api/requisitions/batch/approve).
    
    On Friday, July 21, 2017 at 1:27:20 PM UTC-7, Sebastian Brudziński wrote:

Hi Josh.

            For the first point - this is actually a mistake in RAML. All of those enpoints operate on minified requisitions or UUIDs.

For the second point - that is correct.

Regards,

Sebastian.

          On Fri, Jul 21, 2017 at 10:14 PM, Josh Zamor <josh.zamor@villagereach.org>
          wrote:

Hi Sebastian,

              At a high level the work on validations, not immediately throwing an unchecked exception, etc all sound familiar and on the right course.



              If I'm reading this RAML right, these two statements are true:
  1.                   Batch saving doesn't take in the request a minified Requisition
    
  2.                   Batch approve takes in a set of UUIDs (POSTed) and returns a minifed Requisition
    

Is that right?

              I spent a few minutes tracking down some of these older conversations, so I'll link them here for reference:
  • Technical Committee meeting on batch: https://openlmis.atlassian.net/wiki/x/rwq-Bg
  •                   Dev forum post on batch (and 400 response to partial success): [https://groups.google.com/forum/#!topic/openlmis-dev/day87LiGZro](https://groups.google.com/forum/#%21topic/openlmis-dev/day87LiGZro)
    
    
                    On Wednesday, July 19, 2017 at 8:10:56 AM UTC-7, Sebastian Brudziński wrote:
    

Hello tech committee!

                        During the next couple of days, the Malawi team is planning to work on contributing the batch requisition endpoints from our service ([https://github.com/OpenLMIS-Malawi/mw-requisition-expansion](https://github.com/OpenLMIS-Malawi/mw-requisition-expansion)                            ) to the core requisition service. We would therefore like to confirm technical details for this change and a necessary refactor of the error handling in requisitions.
                        The RAML has already been reviewed on May, during the first attempt to contribute those, although some things have changed since then, so I have uploaded it for review again. Please do take a look: [https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c](https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c)

                        Specifically:

                         - minified DTOs are used in all of those endpoints and they contain only the data necessary for the batch approval page. Working with several requisitions, each having a few hundred products made requests and responses really huge and so it proved necessary to satisfy slow connections

                         - a new endpoint has been introduced to also retrieve requisitions in batch (next to save & approve). It takes an array of UUIDs and will ignore UUIDs that do not exist and UUIDs of requisitions the user does not have access to

                         - partial success (minimum 1 requisition processed with validation errors) returns error code 400
                        Since the batch endpoints need to always process all passed requisitions, a refactor of error handling is necessary in the requisition service. Currently, most validation checks, including permission verification, rely on unchecked exceptions. Throwing an unchecked exception, however, ceases the execution and would therefore lead to the batch endpoints not processing all passed requisitions upon a validation error. Catching unchecked exception was not accepted as viable option by the technical committee and a refactor of error handling was suggested.
                        As a part of this contribution we are therefore planning to re-work the validation checks in the requisition service, such that they do no longer immediately answer with an exception, but rather return an instance of "ValidationResult" class. This new class would store an array of error Message objects and would contain some utility methods to retrieve the errors and to check if the validation passed. It will be up to an endpoint then what to do with that error. Current requisition endpoints will translate the validation messages to exceptions, while the new batch endpoints will store the errors and keep on processing all requisitions.
                        Does all of this sound/look good to you? If you have any further questions or concerns, please let me know as well.

Best regards,

                        Sebastian.


Sebastian Brudziński

                                                          Software Developer / Team Leader

                           sbrudzinski@soldevelo.com

**

                    SolDevelo** Sp. z o.o. [LLC] / [www.soldevelo.com](http://www.soldevelo.com)

                  Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland

                  Phone: +48 58 782 45 40
                  / Fax: +48 58 782 45 41
                --

                You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.

                To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

                To post to this group, send email to openlmis-dev@googlegroups.com.

                To view this discussion on the web visit [https://groups.google.com/d/msgid/openlmis-dev/34d2dc9d-0372-430b-8bbf-462389afac3f%40googlegroups.com](https://groups.google.com/d/msgid/openlmis-dev/34d2dc9d-0372-430b-8bbf-462389afac3f%40googlegroups.com?utm_medium=email&utm_source=footer).

                For more options, visit [https://groups.google.com/d/optout](https://groups.google.com/d/optout).
      **![](https://lh3.googleusercontent.com/proxy/Kq7icsK3MEQjYLwBW84HwuYjQ8aFuyyirMOzt_ENZ5BgyyRVaFVgsbO-vnqZPEKtkcm1Gs2mKJSDSi59adej7wSt1KiO3u5QJa2SfrMvGRh8cyONHJScEXiljA=w5000-h5000)

          SolDevelo** Sp. z o.o. [LLC] / [www.soldevelo.com](http://www.soldevelo.com)

        Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland

        Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

  You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.

  To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

  To post to this group, send email to openlmis-dev@googlegroups.com.

  To view this discussion on the web visit [https://groups.google.com/d/msgid/openlmis-dev/c3360e42-8e8f-4655-919e-5752a85a08ea%40googlegroups.com](https://groups.google.com/d/msgid/openlmis-dev/c3360e42-8e8f-4655-919e-5752a85a08ea%40googlegroups.com?utm_medium=email&utm_source=footer).

  For more options, visit [https://groups.google.com/d/optout](https://groups.google.com/d/optout).


Sebastian Brudziński

    Software Developer / Team Leader


SolDevelo
Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41
sbrudzinski@soldevelo.com