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

refactor: [TECH-1657] standardize format of redux orgunits #3436

Merged
merged 13 commits into from
Oct 26, 2023
Merged

refactor: [TECH-1657] standardize format of redux orgunits #3436

merged 13 commits into from
Oct 26, 2023

Conversation

superskip
Copy link
Contributor

@superskip superskip commented Oct 17, 2023 •
edited by jira bot
Loading

TECH-1657

I've put some effort into arranging the commits.
Might be a good idea to open the first commit in a separate window for easy reference.

@superskip superskip requested a review from a team as a code owner October 17, 2023 11:05
JoakimSM
JoakimSM reviewed Oct 18, 2023
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.

Been thinking about the file structure and we should look into reorganising a bit (will make another ticket for this). For this PR, my suggestion is to put what you have in redux/organisationUnits into a new folder metadataRetrieval/detailedOrganisationUnit. The files should be renamed accordingly (skip the Redux-part). If you like you can change the Redux reducer/key name also. Main reason being that it's most important what you get, not that it is stored in Redux (implementation detail).

But this looks really good overall @superskip, sound logic all around 👍

src/core_modules/capture-core/redux/organisationUnits/useReduxOrgUnit.js Outdated Show resolved Hide resolved
src/core_modules/capture-core/redux/organisationUnits/useReduxOrgUnit.js Outdated Show resolved Hide resolved
src/core_modules/capture-core/redux/organisationUnits/organisationUnits.actions.js Outdated Show resolved Hide resolved
@superskip
Copy link
Contributor Author

superskip commented Oct 18, 2023 •
edited
Loading

Hey @JoakimSM, thanks for the review!

What better way to describe this type of orgunit than naming it's origin? Maybe what it is compatible with, like RulesEngineCompatibleOrgUnit. But it is supposed to be compatible with more than just the rules engine, so that particular name would be a bit misleading. Adding all other current compatibility dependencies to the name has obvious maintainability issues. Simply DetailedOrgUnit again looks rather generic. Maybe something like TonyOrgUnit (or any other name-like prefix) is the best option. The fact that the object value is cached locally, though, would that be invariably true in the future? If so we should maybe name it after this property. Maybe another property which I also think is sort of true is that it wants to be everybody's "friend". For instance UniversallyCompatibleOrgUnit (too long, potentially misleading) or CompatibleOrgUnit (maybe too generic).

🤔 I like the ring to ReduxOrgUnit, also it has got a name-like prefix and has cached value connotations...

JoakimSM
JoakimSM approved these changes Oct 20, 2023
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.

Good job 👍

@github-actions
Copy link

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

@superskip
Merge branch 'master' into TECH-1657
79ff4fa 
# Conflicts:
#	src/core_modules/capture-core/components/DataEntries/EnrollmentRegistrationEntry/EnrollmentRegistrationEntry.container.js
#	src/core_modules/capture-core/components/DataEntries/TeiRegistrationEntry/TeiRegistrationEntry.container.js
#	src/core_modules/capture-core/components/Pages/Enrollment/EnrollmentPageDefault/EnrollmentPageDefault.container.js
#	src/epics/trackerCapture.epics.js
@superskip
fix: change to useCurrentOrgUnitId after merging in master
ceece60
@superskip superskip merged commit 7158db2 into master Oct 26, 2023
35 of 36 checks passed
@superskip superskip deleted the TECH-1657 branch October 26, 2023 19:41
superskip added a commit that referenced this pull request Oct 27, 2023
@superskip
refactor: [TECH-1657] standardize format of redux orgunits (#3436)
fd0c4b5
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.43.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

simonadomnisoru pushed a commit that referenced this pull request Nov 8, 2023
@superskip @simonadomnisoru
refactor: [TECH-1657] standardize format of redux orgunits (#3436)
583d478
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoakimSM JoakimSM JoakimSM approved these changes

Assignees
No one assigned
Labels
e2e-tests released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@superskip @dhis2-bot @JoakimSM