-
Notifications
You must be signed in to change notification settings - Fork 35
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
Adding related items to plannings and events #2110
base: feature/multiple-events-in-planning
Are you sure you want to change the base?
Adding related items to plannings and events #2110
Conversation
support removing items from "related plannings" field.
before it was expanding an associated event which should not happen
…nning' into dragging-events-and-planning-items-by-tomas
@MarkLark86 If an existing planning item gets drag and dropped as related to an event, I'm adding it using the secondary link. Should it be possible to add coverages to planning items linked using the secondary link type? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, mostly just suggestion about implementing logic around the link config in the front-end
@@ -547,6 +549,58 @@ const uploadFiles = (event) => ( | |||
} | |||
); | |||
|
|||
function getLinkedPlanningItems(eventId: string): Promise<IRestApiResponse<IPlanningItem>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this function might be useful in other places, so it would be good to have this available under IPlanningAPI.events.getLinkedPlanningItems
(to be implemented in client/api/events.ts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be good to just return Array<IPlanningItem>
instead of IRestApiResponse<IPlanningItem>
? The function name states get me the list of Planning items
, or do you see the need for attributes under the IRestApiResponse
as well?
const currentlyLinked: Array<IPlanningItem> = res._items; | ||
|
||
const currentLinkedIds = new Set(currentlyLinked.map((item) => item._id)); | ||
const toLink: Array<IPlanningItem['_id']> = planningIds.filter((id) => currentLinkedIds.has(id) !== true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set.has and Array.includes already return a boolean value, what is the use case for explicitly checking for !== true
?
<Spacer h gap="4"> | ||
{ | ||
showEditButton && ( | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be possible to use a button from the UI-Framework instead?
...this.getCurrentValue(), | ||
{ | ||
_id: id, | ||
link_type: 'secondary', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link_type
here could be different values, depending on the instance config.
We have a PLANNING_EVENT_LINK_METHOD
config available in settings.py. This can be exposed to front-end by adding something like the following in planning/init.py
app.client_config["planning"]["event_link_method"] = get_planning_event_link_method()
And adding event_link_method
to IPlanningConfig and in client/config.ts.
Then based on that config value:
one_primary
- Only 1 Event can be linked, therefor if an Event is already linked this action should not be available, and an error thrown here
- Otherwise the
link_type
would beprimary
many_secondary
link_type
is alwayssecondary
one_primary_many_secondary
link_type
isprimary
if there are no Event's currently linkedlink_type
issecondary
otherwise
* Will return true if there is at least one event that can be added | ||
*/ | ||
export function canAddSomeEventsAsRelatedToPlanningEditor(eventIds: Array<IEventItem['_id']>): boolean { | ||
const editor = planningApi.editor(EDITOR_TYPE.INLINE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we would use that event_link_method
config to add additional clause if this action should be available (i.e. config is one_primary
and currentRelatedEvents is not empty, then return false)
.filter((eventId) => isEventAlreadyAddedAsRelated(eventId) !== true) | ||
.map((eventId): IPlanningRelatedEventLink => ({ | ||
_id: eventId, | ||
link_type: 'secondary', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And same here, link_type
would depend on the config and currently linked Events (if any)
I might need a little more context here, as I'm not sure I understand the scenario entirely. When adding a Coverage to a Planning item, there is no logic involved around any linked Events that alllows or denies adding these Coverages. Am I missing something here, could you maybe explain more please |
STT-67
STT-8
Front-end checklist
memo
orPureComponent
to define new React components (and updates existing usages in modified code segments)lodash.get
with optional chaining and nullish coalescing for modified code segmentssuperdeskApi
)superdesk-ui-framework
andsuperdeskApi
when possible instead of using ones defined in this repository.planningApi
where it is possible to usesuperdeskApi
planningApi
)