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

ME: duplicate local record #942

Merged
merged 4 commits into from
Jul 24, 2024
Merged

ME: duplicate local record #942

merged 4 commits into from
Jul 24, 2024

Conversation

LHBruneton-C2C
Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C commented Jul 17, 2024

Description

This PR introduces the local record duplication feature, from the action menu on each record, in all records tables.

The click interaction on a record to open it has been moved to only the title, to allow a click on the menu.

When clicking on the duplicate action, the app navigates to a new duplicate route, which take an UUID as parameter, to read a local record, save it as a copy draft, then enter edition mode.

Architectural changes

The ui-search results-table component now depends on the MatMenuModule from Angular Material.

Screenshots

image

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

Copy link
Contributor

github-actions bot commented Jul 17, 2024

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

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

Copy link
Contributor

github-actions bot commented Jul 17, 2024

📷 Screenshots are here!

@LHBruneton-C2C LHBruneton-C2C marked this pull request as ready for review July 17, 2024 11:22
@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/duplicate-local-record branch from 2ada540 to 86315aa Compare July 17, 2024 11:46
@coveralls
Copy link

coveralls commented Jul 17, 2024

Coverage Status

coverage: 83.088% (-0.5%) from 83.632%
when pulling 6a2ba38 on ME/duplicate-local-record
into 00c93be on main.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Looking and working great, thanks! I made a few suggestions in the code, especially when generating the XML of the duplicated record.

Also it looks like this PR is rebased on another one, but I'm not sure which. Should we wait for another PR to be merged before merging this one?

Comment on lines 241 to 243
return [record, converter] as [CatalogRecord, BaseConverter<string>]
}),
switchMap(async ([record, converter]) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can probably leave that aside and have everything in the same switchMap :)

switchMap(async ([record, converter]) => {
record.uniqueIdentifier = `TEMP-ID-${Date.now()}`
record.title = `${record.title} (Copy)`
const xml = await converter.writeRecord(record)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The original XML document should be provided to writeRecord so that only the affected parts of the XML are changed, otherwise an completely new XML doc will be generated from scratch.

Suggested change
const xml = await converter.writeRecord(record)
const xml = await converter.writeRecord(record, xml)

<gn-ui-interactive-table-column>
<ng-template #header> </ng-template>
<ng-template #cell let-item>
<button
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you think you could use a gn-ui-button there? to have some hover/focus effects (the stop propagation part will also be handled that way)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had tried, but I'll try again.
Opening the menu didn't seem to work with a gn-ui-button, maybe because of the stop propagation indeed.

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/duplicate-local-record branch from 86315aa to 2899383 Compare July 24, 2024 07:17
@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/duplicate-local-record branch from 2899383 to 6a2ba38 Compare July 24, 2024 07:22
@LHBruneton-C2C LHBruneton-C2C merged commit 613f731 into main Jul 24, 2024
9 checks passed
@LHBruneton-C2C LHBruneton-C2C deleted the ME/duplicate-local-record branch July 24, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants