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 coverage data saving/rendering from embed plan #2143

Merged

Conversation

thecalcc
Copy link
Contributor

@thecalcc thecalcc commented Dec 2, 2024

SDCP-863
SDCP-875

Front-end checklist

  • This pull request is adding missing TypeScript types to modified code segments where it's easy to do so with confidence
  • This pull request is using TypeScript interfaces instead of prop-types and updates usages where it's quick to do so
  • This pull request is using memo or PureComponent to define new React components (and updates existing usages in modified code segments)
  • This pull request is replacing lodash.get with optional chaining and nullish coalescing for modified code segments
  • This pull request is not importing anything from client-core directly (use superdeskApi)
  • This pull request is importing UI components from superdesk-ui-framework and superdeskApi when possible instead of using ones defined in this repository.
  • This pull request is not using planningApi where it is possible to use superdeskApi
  • This pull request is not adding redux based modals
  • In this pull request, properties of redux state are not being passed as props to components; instead, we connect it to the component that needs them. Except components where using a react key is required - do not connect those due to performance reasons.
  • This pull request is not adding redux actions that do not modify state (e.g. only calling angular services; those should be moved to planningApi)

@thecalcc thecalcc requested a review from MarkLark86 December 2, 2024 12:49
@@ -176,7 +176,7 @@ function create(updates: Partial<IEventItem>): Promise<Array<IEventItem>> {
function update(original: IEventItem, updates: Partial<IEventItem>): Promise<Array<IEventItem>> {
return superdeskApi.dataApi.patch<IEventItem>('events', original, {
...updates,
associated_plannings: undefined,
associated_plannings: updates.associated_plannings,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarkLark86 this is the fix I found to be working so we persist the changes, and send the proper data back in the UI.

This is a follow up PR based on these comments - https://sourcefabric.slack.com/archives/C06HDLLA1SP/p1733139297225319

Let me know if that's how things should work, from my side there's no issues I see in the UI, and the changes do work.

Copy link
Contributor Author

@thecalcc thecalcc Dec 2, 2024

Choose a reason for hiding this comment

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

All other changes are just improvements, this is the actual change that's fixing the issues mentioned in the linked tickets

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @thecalcc
associated_plannings is set to undefined because we don't want to store this on the Event item itself (this happens for both create and update actions).

Instead what I would try is to re-populate this field after the response from the server/update action:

.then((response) => {
const events = modifySaveResponseForClient(response);
return planningApi.planning.searchGetAll({

.then((response) => { 
    const events = modifySaveResponseForClient(response);
    
    events[0].associated_plannings = updates.associated_plannings
    return planningApi.planning.searchGetAll({ 

Can you please try that and see if that fixes the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok makes sense, just tried that and it does work

Copy link
Collaborator

@MarkLark86 MarkLark86 left a comment

Choose a reason for hiding this comment

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

Looks fine

@thecalcc thecalcc merged commit 88d221a into superdesk:develop Dec 3, 2024
13 checks passed
thecalcc added a commit to thecalcc/superdesk-planning that referenced this pull request Dec 3, 2024
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.

2 participants