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.

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

fix: [DHIS2-15920] skip duplicates reviews #3629

Merged
merged 2 commits into from
May 29, 2024
Merged

Conversation

simonadomnisoru
Copy link
Contributor

@simonadomnisoru simonadomnisoru commented May 2, 2024

DHIS2-15920

Tech summary

  • skip the duplicates check when no attributes values were filled in

@simonadomnisoru simonadomnisoru marked this pull request as ready for review May 2, 2024 12:36
@simonadomnisoru simonadomnisoru requested a review from a team as a code owner May 2, 2024 12:36
Copy link
Contributor

@eirikhaugstulen eirikhaugstulen left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected! 🎉
However, before you merge; are we sure we want to be able to register TEIs with no attributes at all? It seems like it could potentially create a lot of duplicates that would be hard to track? (It's definitively an implementation error if it is though, just curious.)

Copy link

github-actions bot commented May 7, 2024

@simonadomnisoru
Copy link
Contributor Author

simonadomnisoru commented May 7, 2024

Looks good and works as expected! 🎉
However, before you merge; are we sure we want to be able to register TEIs with no attributes at all? It seems like it could potentially create a lot of duplicates that would be hard to track? (It's definitively an implementation error if it is though, just curious.)

Hey @eirikhaugstulen,
Currently, it is already possible to register a TEI without attributes. This PR only removes the call to check for duplicates because all the TEIs in the program are returned and the request performance is poor.
IMHO it is up to the system admin to correctly configure the required attributes if any. Perhaps @karolinelien knows some implementation use-cases where enrolling a TEI without attributes is the desired behavior.

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

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

Tested successfully on 2.42,2.41.1,2.40.4,2.39.6,2.38.7 versions

@simonadomnisoru simonadomnisoru merged commit 3a93911 into master May 29, 2024
44 checks passed
@simonadomnisoru simonadomnisoru deleted the DHIS2-15920 branch May 29, 2024 16:07
dhis2-bot added a commit that referenced this pull request May 29, 2024
## [100.68.14](v100.68.13...v100.68.14) (2024-05-29)

### Bug Fixes

* [DHIS2-15920] skip duplicates reviews ([#3629](#3629)) ([3a93911](3a93911))
* [DHIS2-17102] edit event navigation ([#3592](#3592)) ([85a6f62](85a6f62))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.68.14 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants