Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: [DHIS2-16322] One Click Transfer #3519

Merged
merged 28 commits into from
Mar 5, 2024
Merged

feat: [DHIS2-16322] One Click Transfer #3519

merged 28 commits into from
Mar 5, 2024

Conversation

eirikhaugstulen
Copy link
Contributor

@eirikhaugstulen eirikhaugstulen commented Jan 30, 2024 •
edited
Loading

TECH-summary:

  • Follows the design in our design documents
  • Added a new button for transfers (opens a modal)
  • Fetches the users Search scope (but fallbacks to capture scope if there are no search)

@eirikhaugstulen eirikhaugstulen marked this pull request as ready for review January 30, 2024 08:47
@eirikhaugstulen eirikhaugstulen requested a review from a team as a code owner January 30, 2024 08:47
simonadomnisoru
simonadomnisoru reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

@eirikhaugstulen, can you add some Cypress tests for the ownership transfer functionality? Thank you!

@eirikhaugstulen
fix: pr-review
f0b7ad3
@eirikhaugstulen
fix: errorHandling
e5dfd0f
@eirikhaugstulen
fix: change useEffect to callback handler
576f5b2
@eirikhaugstulen
Merge remote-tracking branch 'origin/master' into eh/feat/DHIS2-16322…
a58e77f 
…_OneClickTransfer

# Conflicts:
#	i18n/en.pot
#	src/core_modules/capture-core-utils/featuresSupport/support.js
#	src/core_modules/capture-core/components/Pages/Enrollment/EnrollmentPageDefault/EnrollmentPageDefault.component.js
#	src/core_modules/capture-core/components/Pages/EnrollmentAddEvent/EnrollmentAddEventPageDefault/EnrollmentAddEventPageDefault.component.js
#	src/core_modules/capture-core/components/Pages/EnrollmentEditEvent/EnrollmentEditEventPage.component.js
@eirikhaugstulen
chore: add prop to enrollment widget config
89df8aa
@eirikhaugstulen
Merge remote-tracking branch 'origin/master' into eh/feat/DHIS2-16322…
0d65fde 
…_OneClickTransfer
@eirikhaugstulen
chore: add live tests
d25160e
superskip
superskip reviewed Feb 7, 2024
View reviewed changes
Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

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

Found a couple of things in addition to the code comments:

  • The app crashes whenever the same orgunit search text used a second time (found it while checking if the caching of search results had any noticeable performance impact).
  • You have added a new fileEnrollmentPageDefault.component.js, which is empty.

...odules/capture-core/components/WidgetEnrollment/TransferModal/hooks/useTransferValidation.js Show resolved Hide resolved
...odules/capture-core/components/WidgetEnrollment/TransferModal/hooks/useTransferValidation.js Show resolved Hide resolved
...re-core/components/WidgetEnrollment/TransferModal/OrgUnitField/useSearchScopeWithFallback.js Show resolved Hide resolved
...re-core/components/WidgetEnrollment/TransferModal/OrgUnitField/useSearchScopeWithFallback.js Outdated Show resolved Hide resolved
...apture-core/components/WidgetEnrollment/TransferModal/OrgUnitField/OrgUnitField.component.js Show resolved Hide resolved
...odules/capture-core/components/WidgetEnrollment/Actions/Transfer/hooks/useUpdateOwnership.js Outdated Show resolved Hide resolved
@eirikhaugstulen
Copy link
Contributor Author

Thanks for the feedback @superskip - should be fixed now!

simonadomnisoru
simonadomnisoru approved these changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

LGTM!

superskip
superskip reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@superskip superskip left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Bug resolved! ⭐
The search goes smoother when hitting a cached result 👍
When not hitting a cached result the search field loses focus though, possibly because the component unmounts. Probably a bit annoying for the users, so I think we should try to fix that as well before moving on.

...odules/capture-core/components/WidgetEnrollment/TransferModal/hooks/useTransferValidation.js Outdated Show resolved Hide resolved
superskip
superskip reviewed Feb 20, 2024
View reviewed changes
Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

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

Thanks @eirikhaugstulen, it looks and works very well now.
I just have one final request: to remove the two lines of code that you see in the comment below 👇

...re-core/components/WidgetEnrollment/TransferModal/OrgUnitField/useSearchScopeWithFallback.js Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 27, 2024 •
edited
Loading

🚀 Deployed on https://deploy-preview-3519--dhis2-capture.netlify.app

@superskip superskip self-requested a review February 27, 2024 17:11
superskip
superskip approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

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

(I put the test for breaking the glass back in today, but it seems it isn't working, feel free to comment it out if it is making trouble for you..)

JoakimSM
JoakimSM reviewed Mar 1, 2024
View reviewed changes
Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

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

A few last minute comments... one bug we should definitely fix

...ules/capture-core/components/WidgetEnrollment/TransferModal/InfoBoxes/InfoBoxes.component.js Outdated Show resolved Hide resolved
src/core_modules/capture-core/components/WidgetEnrollment/Actions/Actions.container.js Outdated Show resolved Hide resolved
...odules/capture-core/components/WidgetEnrollment/Actions/Transfer/hooks/useUpdateOwnership.js Show resolved Hide resolved
@eirikhaugstulen
Copy link
Contributor Author

Thanks @JoakimSM! Implemented all changes.

JoakimSM
JoakimSM approved these changes Mar 1, 2024
View reviewed changes
Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

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

Looks great!

geethaalwan
geethaalwan approved these changes Mar 4, 2024
View reviewed changes
Copy link

@geethaalwan geethaalwan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested successfully on 2.41,2.40.4,2.39.6,2.38.7 versions

@eirikhaugstulen eirikhaugstulen requested review from a team as code owners March 4, 2024 15:40
@eirikhaugstulen eirikhaugstulen merged commit b115ee8 into master Mar 5, 2024
36 of 37 checks passed
@eirikhaugstulen eirikhaugstulen deleted the eh/feat/DHIS2-16322_OneClickTransfer branch March 5, 2024 01:19
dhis2-bot added a commit that referenced this pull request Mar 5, 2024
@dhis2-bot
chore(release): cut 100.59.0 [skip release]
7948025 
# [100.59.0](v100.58.0...v100.59.0) (2024-03-05)

### Features

* [DHIS2-16322] One Click Transfer ([#3519](#3519)) ([b115ee8](b115ee8))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.59.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

@superskip superskip superskip approved these changes

@geethaalwan geethaalwan geethaalwan approved these changes

@JoakimSM JoakimSM JoakimSM approved these changes

@simonadomnisoru simonadomnisoru simonadomnisoru approved these changes

Assignees
No one assigned
Labels
released testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eirikhaugstulen @dhis2-bot @simonadomnisoru @JoakimSM @geethaalwan @superskip