Paweł Gesek

Separation of jobs might help to some extent, but the Java job will still depend on the conversion job that uses npm, since the docs are embedded as part of the Jar. The process of building a servi...

Separation of jobs might help to some extent, but the Java job will still depend on the conversion job that uses npm, since the docs are embedded as part of the Jar. The process of building a service will also get more complex.

As far as the RAML console goes, good news is that I see improvement since I last visited its repo (https://github.com/mulesoft/api-console) - they actually now have a working demo there plus some screenshots. Using this console definitely makes more sense than doing the conversion - I am well aware of the multiple limitations and additional complexity coming from it.

That being said, from what I see it is a Javascript app packaged with node. So we might run into this git issue again. Is the plan to have the RAML console packaged with services (as Swagger is currently) or as a separate standalone that's part of the distro? The latter will make the jar build process no longer deal with any of the javascript/git/npm stuff.

On a side note - how realistic is this issue with the git directory becoming incompatible? Does that happen often with git? I don't really recall many version issues between git installations.

I wouldn't necessarily call this a 'hack' to get around an 'issue'. From my understanding a dependency that is a git repository is perfectly legal with npm - although it obviously has a lot of pitf...

I wouldn't necessarily call this a 'hack' to get around an 'issue'. From my understanding a dependency that is a git repository is perfectly legal with npm - although it obviously has a lot of pitfalls. From what I understand one of the dependencies changed to this form hence the issue.

By the way, the dev-ui image does have git installed - https://github.com/OpenLMIS/dev-ui/blob/master/Dockerfile#L8 . Wonder how many UI dependencies require it? And if it is bad to have git here, I don't see why it is ok to have it there.

If you absolutely don't want git, a workaround is to host a tarball somewhere and override the dependency with shrinkwrap or something. Getting rid of Swagger seems a bit extreme, unless this conversion to the RAML console is something we really want to invest time in.

I think it would be a good idea to handle a cast error here (use instanceof), seems like a more common error that can be made. It should also usually result in a 400, since from my understanding th...

I think it would be a good idea to handle a cast error here (use instanceof), seems like a more common error that can be made. It should also usually result in a 400, since from my understanding this will happen when clients ask to expand a bad field

Ok, I get it, we want to treat null as a unique value

Ok, I get it, we want to treat null as a unique value

Doesn't the unique index ignore null values? Do tests fail if you remove lines 6-8?

Doesn't the unique index ignore null values? Do tests fail if you remove lines 6-8?

builder was added

builder was added

OLMIS-3087: Rename physical_inventory_line_item_reasons
OLMIS-3087: Rename physical_inventory_line_item_reasons
OLMIS-3087: Rename physical_inventory_line_item_reasons

* Rename physical_inventory_line_item_reasons to

physical_inventory_line_item_adjustments

* Add foreign keys

* Rename the StockAdjustment class to PhysicalInventoryLineItemAdjustment

    • -0
    • +23
    /src/main/resources/db/migration/20171110113906384__rename_physical_inventory_line_item_reasons_and_add_keys.sql
since these are primitives (ints) I think it's fine to use the boolean approach here, rather than boilerplate it. What do you think Josh Zamor?

since these are primitives (ints) I think it's fine to use the boolean approach here, rather than boilerplate it. What do you think Josh Zamor?

Ok looks good to me now

Ok looks good to me now

and UUID strings as values for that key.

and UUID strings as values for that key.

OLMIS-3453: remove the index from POD line item - > orderlineitemid

I think Łukasz Lewczyński is confusnig the orderid with orderlineitemid. I also don't we need this index - Paweł Albecki I suggest we drop it, unless you know of where it can be used.

I think Łukasz Lewczyński is confusnig the orderid with orderlineitemid. I also don't we need this index - Paweł Albecki I suggest we drop it, unless you know of where it can be used.

The naming of these methods is wrong: they should not start with provide, but something like 'supports'. However I believe you used 'provide' because you followed Josh's pattern, without fully impl...

The naming of these methods is wrong: they should not start with provide, but something like 'supports'. However I believe you used 'provide' because you followed Josh's pattern, without fully implementing it. From my understanding the method should be called 'provideStatusChangeExporter()' and it should return an exporter for a status change - i.e. new StatusChangeDto. If the dto does not support this field, it should return null (Josh mentioned using the new and hip Optional).

I think this is almost what we want, but not there yet, the exporters should be extended with methods like addStatusChange(StatusChangeExporter) and the provide method should be refactored to return an instance of the exporter or null (wrap it in an Optional).

Correct me if I'm wrong Josh Zamor.

Looks ok, but as I mentioned it does not fully follow the pattern. Some tweaking is needed.

Looks ok, but as I mentioned it does not fully follow the pattern. Some tweaking is needed.

Josh Zamor I've went through this review and resolved most of your comments (expect one). Can you take a look at that one and let us know if you want to be involved in resolving it. Most of your c...

Josh Zamor I've went through this review and resolved most of your comments (expect one). Can you take a look at that one and let us know if you want to be involved in resolving it.

Most of your comments are resolved through the new review that introduces the pattern you suggested, so lets continue there.

Paweł Albecki when I look at this code and javadoc, it is not immediately clear to me why one method is using a map, while the other is using a multimap. I think that's also what Josh Zamor is stru...

Paweł Albecki when I look at this code and javadoc, it is not immediately clear to me why one method is using a map, while the other is using a multimap. I think that's also what Josh Zamor is struggling here with. Can we make it more clear what is the difference between these two methods?

Josh Zamor I see you signed yourself out from the review mentioned. Can we resolve this comment? Paweł Albecki I'm not following what is happening with these different reviews - can we keep referr...

Josh Zamor I see you signed yourself out from the review mentioned. Can we resolve this comment?

Paweł Albecki I'm not following what is happening with these different reviews - can we keep referring to other reviews to a minimum?

Paweł Albecki not sure why it is in a separate review, given how small a change it is - in the future try to include such small changes in the review that has the comment. Anyway I checked the oth...

Paweł Albecki not sure why it is in a separate review, given how small a change it is - in the future try to include such small changes in the review that has the comment.

Anyway I checked the other review and the method in fact never returns null. The signature was changes as Josh Zamor suggested, resolving.

Same as with others, lets continue in the separate review. Resolving

Same as with others, lets continue in the separate review. Resolving

Marking as resolved, let's continue in the separate review

Marking as resolved, let's continue in the separate review

I verified approvedRequisitionDto has no such field as program Id. I assume it's because a batch screen only works in the context of one program. I am marking this as resolved.

I verified approvedRequisitionDto has no such field as program Id. I assume it's because a batch screen only works in the context of one program. I am marking this as resolved.

Marking as resolved, lets continue in the next review

Marking as resolved, lets continue in the next review

same comment

same comment

same comment as in previous class

same comment as in previous class