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

[ME]: Clean up attachments on publishing #1004

Merged
merged 2 commits into from
Oct 7, 2024
Merged

[ME]: Clean up attachments on publishing #1004

merged 2 commits into from
Oct 7, 2024

Conversation

rcaplier
Copy link
Collaborator

This PR add an effect cleanRecordAttachments on saveRecordSuccess action that delete all ressources that are not used in the record.

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

This work is sponsored by.

@rcaplier rcaplier self-assigned this Sep 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 25, 2024 •
edited
Loading

Affected libs: api-repository, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, feature-editor, feature-auth, common-domain, api-metadata-converter, ui-search, common-fixtures, ui-elements, feature-notifications, ui-catalog, util-shared, ui-widgets, ui-inputs, ui-layout, ui-map, ui-dataviz,
Affected apps: metadata-editor, datahub, demo, webcomponents, map-viewer, search, datafeeder, metadata-converter, data-platform,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@rcaplier rcaplier force-pushed the me-clean-up-attacments-on-publishing branch from 1d14433 to 7156342 Compare September 25, 2024 12:47
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 25, 2024 •
edited
Loading

📷 Screenshots are here!

@rcaplier rcaplier force-pushed the me-clean-up-attacments-on-publishing branch 2 times, most recently from e49a41a to 6205ee3 Compare September 26, 2024 13:43
@coveralls
Copy link

coveralls commented Sep 26, 2024 •
edited
Loading

Coverage Status

coverage: 82.968% (-0.7%) from 83.65%
when pulling 4ee2cf9 on me-clean-up-attacments-on-publishing
into 8237040 on main.

LHBruneton-C2C
LHBruneton-C2C approved these changes Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C left a comment

Choose a reason for hiding this comment

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

Looking good and tested OK, thanks!
I just have a few questions, but for my own understanding.

libs/api/repository/src/lib/gn4/platform/gn4-platform.service.ts Show resolved Hide resolved
libs/feature/editor/src/lib/+state/editor.effects.spec.ts Outdated
Comment on lines 176 to 182
it('dispatch markRecordAsChanged', () => {
actions = hot('-a-|', {
a: EditorActions.saveRecordSuccess(),
})
const expected = hot('---|')
expect(effects.cleanRecordAttachments$).toBeObservable(expected)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get this test... Is it the proper it description?
Also, the way I read the marble test, no emit is expected on the observable. Is it really what we want, or am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to change the it after the copy pasta. The observable returns void so I don't know if I can marble that differently ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem here is that no event is different from an event with no content. Searching on SO, most people expect an undefined tick: https://stackoverflow.com/questions/64258427/how-do-i-marble-test-an-observable-of-void-values

@rcaplier rcaplier force-pushed the me-clean-up-attacments-on-publishing branch from 6205ee3 to 8b83fc7 Compare September 30, 2024 07:14
feat(me): Added an effect cleanRecordAttachments on saveRecordSuccess…
c7618f4 
… action that delete all ressources that are not used in the record.
@rcaplier rcaplier force-pushed the me-clean-up-attacments-on-publishing branch 2 times, most recently from 5ae18ca to 4bccb25 Compare September 30, 2024 18:22
@rcaplier rcaplier force-pushed the me-clean-up-attacments-on-publishing branch from 4bccb25 to 4ee2cf9 Compare September 30, 2024 19:10
@rcaplier rcaplier merged commit a9a88e9 into main Oct 7, 2024
13 checks passed
@rcaplier rcaplier deleted the me-clean-up-attacments-on-publishing branch October 7, 2024 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LHBruneton-C2C LHBruneton-C2C LHBruneton-C2C approved these changes

Assignees

@rcaplier rcaplier

Labels
tech task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rcaplier @coveralls @LHBruneton-C2C