Skip to content

FINERACT-1306 fix Reporting meta-data entry not found"#1620

Closed
francisguchie wants to merge 10 commits intoapache:developfrom
francisguchie:FINERACT-1306reportingMetaData
Closed

FINERACT-1306 fix Reporting meta-data entry not found"#1620
francisguchie wants to merge 10 commits intoapache:developfrom
francisguchie:FINERACT-1306reportingMetaData

Conversation

@francisguchie
Copy link
Contributor

Description

Issue here FINERACT-1306

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per https://github.com/apache/fineract/#pull-requests

  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.

  • Create/update unit or integration tests for verifying the changes made.

  • Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.

  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes

  • Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

@vorburger
Copy link
Member

I don't understand yet how this will fix FINERACT-1306... isn't this an "empty" report? Why is this required?

@francisguchie
Copy link
Contributor Author

@vorburger

after commit no , we get two errors

  1. reportCategoryList.not.found
    for API : /fineract-provider/api/v1/runreports/reportCategoryList?R_reportCategory=Fund&genericResultSet=false&parameterType=true[&tenantIdentifier=default|https://localhost:8443/fineract-provider/api/v1/runreports/FullReportsList?genericResultSet=false&parameterType=true&tenantIdentifier=default]

and
2. fullReportList.not.found
for: API /fineract-provider/api/v1/runreports/FullReportsList?genericResultSet=false&parameterType=true&tenantIdentifier=default).

When we add these two reports (me & @rrpawar96 ) with names "ReportCategoryList" and "FullReportList"
these 2 errors do not show up.

Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

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

I'm quite reluctant to accept merging this one; as per FINERACT-1306, this "just" adds x2 "fake" reports, to "work around" something I'm not sure I really agree this is a regression that we should work around... The error "just" means that /runreports could not find a report named "reportCategoryList". To me, that is the the expected behavior and it's therefore "working as intended" (WAI) & just fine - I don't see the problem. Do you?

@ptuomola
Copy link
Contributor

ptuomola commented Mar 9, 2021

@vorburger I haven't looked at this in detail, but what I think may be happening: the two reports (reportCategoryList and FullReportList) are present in the table stretchy_parameter but NOT in stretchy_report.

It may be that before your refactoring, this was ok - i.e. the code did not check stretchy_report and was happy if the entry was present in stretchy_parameter. It may be that we are now a bit stricter and require an entry in both? Hence this would "fix" the issue by ensuring these two reports are present in both tables.

I'm not sure why this PR contains a change to the Spring Framework dependency though.

@vorburger
Copy link
Member

I see. #1663 is basically this PR, but "cleaned up", and with a test. @francisguchie would you be willing to close this in favor of that?

@francisguchie
Copy link
Contributor Author

I see. #1663 is basically this PR, but "cleaned up", and with a test. @francisguchie would you be willing to close this in favor of that?

Thanks @vorburger for #1663 I am closing this

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.

4 participants