Comment Search

Defect Metrics

Statistics on the results
Ranking
Defect Report
Classification
Defect Report
  • 808 Defects:
  • 151 major defects, and
  • 657 other defects
  • from 21790 comments

Search Criteria

Search for comments

Search Criteria

Project
Comment content
Review PermaId
After
Before
Comment Author
Search Type
Search Type:
Review State
Review State:
Metrics:
Metric Ranking
Metric Classification
Submit
  •  

Comment Results

Review Name Created Custom Fields Content
FEOLMIS-1 06 Jun 2016

this line here, truly genius
FEOLMIS-1 06 Jun 2016

this line here was clearly made by an idiot
FEOLMIS-1 06 Jun 2016 Classification: Editorial Ranking: Minor

What were you thinking?
FEOLMIS-1 06 Jun 2016

But will it scale?
FEOLMIS-1 06 Jun 2016

But I wrote that!
FEOLMIS-1 06 Jun 2016

Instead of:

  requisition-build:
      GRADLE_OPTS: '-Dorg.gradle.daemon=false'


we should do:

  requisition-build:
      GRADLE_OPTS: '-Dorg.gradle.daemon=true'
FEOLMIS-1 06 Jun 2016 Ranking: Major Classification: Inconsistent

These are my general comments. Indicating a defect.
FEOLMIS-1 06 Jun 2016

I'd say check the "blame" button but then it says it was Chongsun... I know I added this block before the service was re-named. What was I thinking!? Ugh the textual hate I can convey in this text area widget!!!!
FEOLMIS-1 06 Jun 2016

Like, fishes or music?
FEOLMIS-1 06 Jun 2016

Indiana Jones, mostly.
FEOLMIS-1 06 Jun 2016

Now what do we do? Close it?
FEOLMIS-5 06 Jun 2016

Please review this!
FEOLMIS-8 14 Jun 2016

Klaudia doesn't have access here yet, so I created the review for her.
FEOLMIS-8 16 Jun 2016

Couple questions:
1. if I'm using it correctly, I can't seem to trigger the errors through `gradle check`, `gradle pmdMain` or `gradle pmdIntegrationTest`. That is, I added duplicate imports and unused private fields and no errors popped up in `build/reports/pmd/`
2. does Klaudia now have access to Crucible?
FEOLMIS-8 17 Jun 2016

1. I'm trying to fix that bug.
2. Yes, I have access.
FEOLMIS-8 21 Jun 2016

Looks like more code was added related to PMD, does this review need to be updated?
FEOLMIS-8 22 Jun 2016

Changes have been updated.
FEOLMIS-8 24 Jun 2016

Looks great, I did add the unused import rule as well as it seems simple and useful. Thanks!
FEOLMIS-10 27 Jun 2016

Does transifex need to occur for Sonar to provide an analysis? It seems like we might be able to reduce some of the configuration if we didn't need to notify transfiex. Then we could add to the docker-compose.builder.yml instructions for interacting with our sonarqube server.
FEOLMIS-10 28 Jun 2016

Transifex does not need to occur for Sonar to execute an anaysis. I removed sonar.sh file and configured sonar in docker-compose.builder.yml file.
FEOLMIS-10 28 Jun 2016

Looks great, thanks. I think we can close this one if we think no more needs to be added to the readme in terms of running sonar through docker compose?
FEOLMIS-11 16 Jun 2016

Is there a corresponding change to the id needed in the OrderLine? I still see Integer.
FEOLMIS-11 17 Jun 2016

Jakub Kondrat
FEOLMIS-11 21 Jun 2016

Does this need to be updated?
FEOLMIS-11 27 Jun 2016

updated with new commit
FEOLMIS-12 16 Jun 2016

You may want to consider sticking with Jackson as I believe that Spring Boot here is implicitly using it. It may make a difference if we start using Jackson Projections.
FEOLMIS-12 17 Jun 2016

Jakub Kondrat
FEOLMIS-12 29 Jun 2016

updated with latest changes
FEOLMIS-13 16 Jun 2016

Should also be unique
FEOLMIS-14 16 Jun 2016

Consider Jackson here as I believe it's already bundled.
FEOLMIS-14 17 Jun 2016

Jakub Kondrat
FEOLMIS-16 20 Jun 2016

Looking great, couple questions:
1. should we incorporate nginx into the typical developer experience (here being developing one service), instead of a separate compose step?
2. We should probably add this to the service-template


Also FYI I forked/cloned this one to openlmis on github just so we have the code and know the license. We should likely establish this as a best practice.
FEOLMIS-16 21 Jun 2016

Josh Zamor
1. Developing with nginx would require us to build deployment image each time a change is made, I think it's best to keep the dev environment as is
2. Sure, I'm gonna add it shortly
FEOLMIS-16 27 Jun 2016

added service-template changes
FEOLMIS-16 27 Jun 2016

Jakub Kondrat, please see this PR I sent your way: https://github.com/OpenLMIS/openlmis-requisition/pull/10

This is what I was thinking of when I asked if we could "integrate" it into the dev environment. It could be useful here as it's closer to production - security will also likely be done here (nginx) and might be applicable in development in a service. Let me know if this works for you and if you think it'd be a hindrance or a help. Thanks.
FEOLMIS-16 28 Jun 2016

Josh Zamor Is there any way to start openlmis/dev and open service ports for nginx at the same time? I can't get the proxy running on port 80 when using config from the PR
FEOLMIS-16 28 Jun 2016

Hi Jakub Kondrat, it works here but we're using Docker 1.12. In the PR, I changed the docker compose so that the nginx proxy was linked to the service. This creates a (fake) dependency which will bring up nginx, like it does postgres, when you run docker-compose run --service-ports requisition.
FEOLMIS-16 29 Jun 2016

Josh Zamor It makes sense now, I'm merging the PR
FEOLMIS-17 27 Jun 2016

Thanks, we could just add this work to the other review so that it was all under one review. Will mark complete as FEOLMIS-10 captures it.
FEOLMIS-19 27 Jun 2016

How does a non-OpenLMIS developer use this to build the service? It seems like we've made a presumption that a user will have credentials to get the translations since there is one combined process: push messages and pull new translations. I would think that to be good open source citizens, we'd need to support anyone building one of our services, regardless if they have Transifex credentials. To do this I think we'd need to:

  • state that our policy for all services is that the translations for all services (governed by us) are of an appropriate license and are publicly downloadable by anyone
  • the default/convention in the build process, should it not have credentials to transifex, is to only attempt the download of the latest translations
FEOLMIS-19 27 Jun 2016

Unfortunately it appears that the Transifex client does not allow interacting with the Transifex server (including pulling of resources) without authentication credentials. So, how will others be able to access these resources?
FEOLMIS-19 27 Jun 2016

Transifex accounts are free, and open source projects are free, so I don't think it is too high of a bar for one to acquire an account/credentials.
FEOLMIS-20 27 Jun 2016

Did we decide to move away from SDR? I know we've discussed it, but it doesn't look like there's logic here that SDR wouldn't give you? Thanks.
FEOLMIS-20 12 Jul 2016

Josh Zamor We had to make a custom mapping so that it is only possible to create and not edit programs
FEOLMIS-22 29 Jun 2016

I feel like we shouldn't be setting the response status here, but throw an exception and have the controller advice/exception handler return a correct status code.
FEOLMIS-22 29 Jun 2016

Similar to periods, I think we should throw an exception here and have advice controller/exception handler take care of this.
FEOLMIS-22 29 Jun 2016

should be private
FEOLMIS-22 29 Jun 2016

should be private
FEOLMIS-22 29 Jun 2016

should be private
FEOLMIS-22 29 Jun 2016

I suppose we are interested in last period in the meaning of the endDate. Can we guarantee that the retrieved periods are sorted by the date and that the last element in the iterator is actually the latest one?