Skip to content

REPORT-806 : Upgraded RunReportListController and RunReportFormController to utilize annotations#175

Open
dilanthas wants to merge 1 commit into
openmrs:masterfrom
dilanthas:REPORT-806
Open

REPORT-806 : Upgraded RunReportListController and RunReportFormController to utilize annotations#175
dilanthas wants to merge 1 commit into
openmrs:masterfrom
dilanthas:REPORT-806

Conversation

@dilanthas

@dilanthas dilanthas commented May 25, 2019

Copy link
Copy Markdown

Ticket : https://issues.openmrs.org/browse/REPORT-806

What i have done : RunReportListController and RunReportFormController were using depricated SimpleFormController. i have modified the two controllers to use annotations.

@mks-d mks-d requested a review from dkayiwa June 19, 2019 22:03
import org.springframework.validation.ValidationUtils;
import org.springframework.validation.Validator;

import java.util.*;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkayiwa This class was removed as the validator logic added back to the controller

@dkayiwa

dkayiwa commented Jun 20, 2019

Copy link
Copy Markdown
Member

@dilanthas are all changes in this pull request required because of a mere switch to using annotations?

@dilanthas

dilanthas commented Jun 21, 2019

Copy link
Copy Markdown
Author

@dilanthas are all changes in this pull request required because of a mere switch to using annotations?
@dkayiwa I did two extra changes by moving the command object class and validator outside from the controller

@dkayiwa

dkayiwa commented Jun 21, 2019

Copy link
Copy Markdown
Member

It is easier to review pull requests that do just one thing. For instance, any improvements that are not related to annotations can be done using a separate ticket and pull request. https://opensource.com/article/18/6/anatomy-perfect-pull-request

@dilanthas

Copy link
Copy Markdown
Author

It is easier to review pull requests that do just one thing. For instance, any improvements that are not related to annotations can be done using a separate ticket and pull request. https://opensource.com/article/18/6/anatomy-perfect-pull-request

@dkayiwa I have added back the validation logic into the controller. But the other change (moving the CommandObject outside from the controller ) is required to make this work. It cannot be a subclass of the Controller otherwise it will give error Failed to instantiate [org.openmrs.module.reporting.web.reports.RunReportFormController$CommandObject]: No default constructor found

@dilanthas

Copy link
Copy Markdown
Author

@dkayiwa Extra things were rolled back. Now it only contains the changes related to the jira

@dilanthas

Copy link
Copy Markdown
Author

@dkayiwa anything else need to be fixed in order to merge this ?

@dilanthas

Copy link
Copy Markdown
Author

@mseaton Will you be able to review this please ?

@gracepotma

Copy link
Copy Markdown

@dkayiwa or @mseaton can this be merged or closed?

import org.springframework.web.servlet.mvc.BaseCommandController;
import org.springframework.web.servlet.mvc.SimpleFormController;
import org.springframework.web.servlet.view.RedirectView;
import org.springframework.validation.*;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dilanthas do you still have time to replace the wildcard import as advised at? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotusewildcardimports

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants