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

[PRO-284] fix support sync component internal tags #102

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

alvarosabu
Copy link
Contributor

@alvarosabu alvarosabu commented Jul 2, 2024

Pull request type

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Other (please describe):

How to test this PR

  • Go to vscode
  • Run & Debug tab
  • Run "Launch CLI Script"
  • Check result on debug console.
  • (Optional) Check the spaces directly (295017, 295018)

What is the new behavior?

  • Added internal logic to handle component internal tags (Will only create tags on target if they don't exist yet)
  • Tested creation of new component with tags if it doesn't exist on target space
  • Tested update of component with tags if it already exists on target space

Other information

I had to force the updated component internal_tag_ids to always be like the source component because the current solution mergeComponents is not recursive and is not merging the array correctly. I don't think is worth to refactor mergeComponents right now since we plan other things for this package in the near future.

 payload.component.internal_tag_ids = sourceComponentData.internal_tag_ids;
updateComponent(spaceId, componentId, sourceComponentData, targetComponentData) {
    const payload = {
      component: this.mergeComponents(
        sourceComponentData,
        targetComponentData
      )
    };
    payload.component.internal_tag_ids = sourceComponentData.internal_tag_ids;
    return this.client.put(`spaces/${spaceId}/components/${componentId}`, payload).then((response) => {
      const component = response.data.component || {};
      return component;
    }).catch((error) => Promise.reject(error));
  }

@alvarosabu alvarosabu added the bug Something isn't working label Jul 2, 2024
@alvarosabu alvarosabu self-assigned this Jul 2, 2024
Copy link

Copy link
Contributor

@Edo-San Edo-San left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Agreed with your take on the refactoring of the mergeComponents function: that's probably not worth it right now.

As for the tests, well.. They should be heavily refactored alongside the rest of this repo, so I wouldn't bother with them as for now 😅 My 2 cents.

Thanks for this PR @alvarosabu 🙏 MUCH appreciated! I love tags as opposed to folders, even tho the support is a bit lackluster as for now, but they're a new feature and I'm looking forward to use them more and more in the future and eventually replace all the folders with tags!

Copy link
Contributor

@christianzoppi christianzoppi left a comment

Choose a reason for hiding this comment

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

Thanks @alvarosabu! I agree with you and @Edo-San on the comment about the mergeComponents function.

I also tested the function to see if I could spot any edge case, but so far all good. You have done a great job so quickly! 🙏🏻

@christianzoppi christianzoppi merged commit a81c099 into master Jul 4, 2024
1 check failed
@christianzoppi christianzoppi deleted the bugfix/support-sync-component-tags branch July 4, 2024 06:39
Copy link

github-actions bot commented Jul 4, 2024

🎉 This issue has been resolved in version 3.32.2 🎉

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
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants