Skip to content

FINERACT-2541: owner to owner transfer functionality#5638

Merged
adamsaghy merged 2 commits intoapache:developfrom
openMF:FINERACT-2541/owner-to-owner-transfer
Mar 25, 2026
Merged

FINERACT-2541: owner to owner transfer functionality#5638
adamsaghy merged 2 commits intoapache:developfrom
openMF:FINERACT-2541/owner-to-owner-transfer

Conversation

@budaidev
Copy link
Contributor

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

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

  • Write the commit message as per our guidelines
  • 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 our coding conventions.
  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

@budaidev budaidev force-pushed the FINERACT-2541/owner-to-owner-transfer branch from 35ebfd6 to bc12b2e Compare March 17, 2026 04:49
@adamsaghy adamsaghy marked this pull request as ready for review March 19, 2026 09:38
@budaidev budaidev force-pushed the FINERACT-2541/owner-to-owner-transfer branch from bc12b2e to babe420 Compare March 20, 2026 12:10
@ruzeynalov ruzeynalov force-pushed the FINERACT-2541/owner-to-owner-transfer branch 2 times, most recently from 58ada94 to d9cdcbe Compare March 24, 2026 10:53
@ruzeynalov ruzeynalov force-pushed the FINERACT-2541/owner-to-owner-transfer branch from d9cdcbe to 6bf0dee Compare March 24, 2026 12:46
@vidakovic
Copy link
Contributor

@ruzeynalov please squash 2 commits; other than that LGTM

@adamsaghy
Copy link
Contributor

@ruzeynalov please squash 2 commits; other than that LGTM

Community was decided to go with "merge commits" route and allowing multiple commits / PR (if they are not repetitive like fix trailing spaces, etc.)

@vidakovic
Copy link
Contributor

ah, ok... well then

@vidakovic
Copy link
Contributor

@adamsaghy btw: before when I merged a similar one I saw that my user was kind of hijacking the authorship of the PR... how can this be avoided?

@adamsaghy
Copy link
Contributor

@adamsaghy btw: before when I merged a similar one I saw that my user was kind of hijacking the authorship of the PR... how can this be avoided?

Can you link the PR?

@vidakovic
Copy link
Contributor

vidakovic commented Mar 24, 2026

@adamsaghy btw: before when I merged a similar one I saw that my user was kind of hijacking the authorship of the PR... how can this be avoided?

Can you link the PR?

PR: #5662

commit: 2e71832

@adamsaghy
Copy link
Contributor

adamsaghy commented Mar 24, 2026

@adamsaghy btw: before when I merged a similar one I saw that my user was kind of hijacking the authorship of the PR... how can this be avoided?

Can you link the PR?

PR: #5662

commit: 2e71832

Assuming this is the wanted behaviour, since you are the author of the merge commit. Anyway the original commit still authored by Peter, but now there are multiple commits.

@meonkeys right?

@vidakovic
Copy link
Contributor

@meonkeys also: should I just leave then the default messages that is provided to me (was something like "Merge: FINERACT-1234 blablabla", but also repeated the commit/PR titles in the extended description)... looks a bit weird and unnecessary given the already existing commit(s) with description(s). Any recommendations/preferences?

@meonkeys
Copy link
Contributor

meonkeys commented Mar 24, 2026

@adamsaghy wrote:

Assuming this is the wanted behaviour, since you are the author of the merge commit. Anyway the original commit still authored by Peter, but now there are multiple commits.

@meonkeys right?

That's right.

@vidakovic wrote:

@meonkeys also: should I just leave then the default messages that is provided to me (was something like "Merge: FINERACT-1234 blablabla", but also repeated the commit/PR titles in the extended description)... looks a bit weird and unnecessary given the already existing commit(s) with description(s). Any recommendations/preferences?

Aleks, I like what you did for the first line of the log in merge commit 2e71832, and I'd also leave the rest of the autogenerated commit log message below that (in "Extended description").

@adamsaghy adamsaghy merged commit 37a0097 into apache:develop Mar 25, 2026
43 checks passed
@adamsaghy adamsaghy deleted the FINERACT-2541/owner-to-owner-transfer branch March 25, 2026 12:33
@devi-pathak2263
Copy link
Contributor

devi-pathak2263 commented Mar 26, 2026

Hi, I was reviewing this PR and noticed that updating the approved amount with the same value still triggers history creation and business events.

I’ve created a small follow-up improvement to prevent no-op updates by adding validation when the new approved amount equals the current value. Also added integration test coverage for this scenario.

PR: #5696 (follow-up improvement)

Please let me know if this aligns with the intended behavior.

@devi-pathak2263
Copy link
Contributor

Hi, I was reviewing this PR and noticed that updating the approved amount with the same value still triggers history creation and business events.

I’ve created a small follow-up improvement to prevent no-op updates by adding validation when the new approved amount equals the current value. Also added integration test coverage for this scenario.

PR: #5696 (follow-up improvement)

Please let me know if this aligns with the intended behavior.

Just a quick update: based on the feedback, I’ve created a separate JIRA issue for this improvement (FINERACT-2556) and updated the PR accordingly.

PR: #5696

Thanks again for the guidance!

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.

6 participants