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

feat: [DHIS2-16372] Delete Relationships #3520

Merged
merged 8 commits into from
Mar 3, 2024

Conversation

eirikhaugstulen
Copy link
Contributor

Tech-summary:

  • Added a IconDelete to each row in the TEI Relationships widget
  • Show confirmation modal asking the user if they are sure
  • Optimistically delete the row when clicked

Some caveats:

  • Needs update when 3517 is merged
  • Soft-deleted relationships are still returned in the new tracker endpoints. Issue raised in tracker-agile-team

@eirikhaugstulen eirikhaugstulen marked this pull request as ready for review January 30, 2024 15:12
@eirikhaugstulen eirikhaugstulen requested a review from a team as a code owner January 30, 2024 15:12
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 a Cypress test for the delete relationship functionality? Thank you!

@eirikhaugstulen
Copy link
Contributor Author

Hey @simonadomnisoru!
Thanks for the review. I have implemented and written some tests. (These are commented out for now as there is a bug in the API - will fix before merge)

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.

Added a reminder to look into before merging, other than that it looks great! 🎉

cy.wait('@deleteRelationship')
.its('request.body')
.should('deep.equal', {
relationships: [{ relationship: 'mwswG3RMNuu' }],
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder after the bug in the API is fixed: To keep DB consistency, the relationship that was deleted should be added back. Otherwise, the there is an existing relationship step will fail the next time the scenario is run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This data is actually just mocked, so no need to add it again. We intercept the request for relationships in the before hook and then assert that we send the actual call to the api with the correct data! WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Testing against mocked data versus the API and DB has advantages and disadvantages 🤔 . By using mock data, we will no longer benefit from having the latest API changes available. We will need to ensure that the mocks are always up to date, otherwise, we may not detect when we introduce bugs.
Let's discuss it more in-depth in our next FE check-in meeting!

Copy link

github-actions bot commented Feb 2, 2024

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.41,2.40.3,2.39.5,2.38.7 versions

…_DeleteTEIRelationships

# Conflicts:
#	i18n/en.pot
@eirikhaugstulen eirikhaugstulen merged commit 9fed562 into master Mar 3, 2024
37 checks passed
@eirikhaugstulen eirikhaugstulen deleted the eh/feat/DHIS2-16372_DeleteTEIRelationships branch March 3, 2024 07:55
dhis2-bot added a commit that referenced this pull request Mar 3, 2024
# [100.58.0](v100.57.6...v100.58.0) (2024-03-03)

### Features

* [DHIS2-16372] Delete Relationships ([#3520](#3520)) ([9fed562](9fed562))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.58.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants