nidhiii128 commented on PR #6326: URL: https://github.com/apache/fineract/pull/6326#issuecomment-6270310990
> Hi @nidhiii128, thanks for the PR. Docs-only, so I'd normally approve silently, but the example it includes doesn't exist. > > * `validation.adoc` (~L29): the `include::` points at `CalendarCreateRequest.java`, which I can't find on develop (`portfolio/calendar/data/` only has `CalendarData` and `CalendarHistoryDataWrapper`). An include on a missing file breaks or warns in the doc build. Can we point it at a DTO that is actually merged, with a real `@Valid` usage? `lines=30..` also silently breaks as soon as the license header changes, a tagged region is safer. > * The `calendar.title.*` message keys aren't in `ValidationMessages.properties`. Let's quote real keys (e.g. the `organisation.staff.*` ones) so people can grep them and the doc stays verifiably true. > * "JAX-RS aborts the call with 400": which mapper actually produces the 400, and what does the body look like? Link to it or show the response shape. "Clients pattern-match on these strings today": where does that come from? I'd drop it or back it up, it reads like a contract we never promised. > * No newline at end of file. The PR template is untouched, a one-liner description would help, and the PR is stale. Hey @galovics, thanks for the thorough review you were right on all of it. The CalendarCreateRequest reference was leftover from when I assumed my command-migration PRs would land first; that was a bad call for a docs change. Repointed everything at StaffCreateRequest since it's already on develop and wired up with @Valid, and swapped in the real staff.* keys from ValidationMessages.properties. Also rewrote the 400 bit to describe what actually happens instead of naming a mapper that isn't there, dropped the pattern-match line, fixed the EOF newline, and added a PR description. Rebased onto latest develop. Let me know what you think. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
