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

[SDESK-7441] Create basic async resource models and services for planning.events #2131

Merged
merged 16 commits into from
Dec 3, 2024

Conversation

eos87
Copy link

@eos87 eos87 commented Nov 20, 2024

Purpose

Implement the basic async resource and service for Planning Events app

What has changed

  • Added resources models and services for Events, Planning, Assignments and Published
  • Fixed pytests so they work with async
  • Partially fixed behave tests (~82% of them now green)
  • Updated GitHub actions to run with Python v3.10 and latest actions

Note

This PR depends on superdesk/superdesk-core#2758. Once that PR is merged, I will update the requirements here to point to superdesk-core async branch instead.

Thanks for checking!

@eos87 eos87 requested a review from MarkLark86 November 20, 2024 16:29
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.

@eos87 I've had a quick look over it, looks fine so far. Will re-review once you've completed the schema changes etc

Fix behave tests partially

SDESK-7441

Allow behave tests to run async code

SDESK-7441

Fix pytests and use python 3.10 only

Disable some actions and add verbose mode 999

Remove python 3.8

Point sd core to fix branch

Revert "Fix linter issues"

This reverts commit 152cfb5.

Revert changes to ci-install

SDESK-7441

Fix first batch of tests

Reapply "Fix linter issues"

This reverts commit e5ac69a.

Fix second batch of tests

SDESK-7441

Fix tests batch 3

Fix tests batch 4

SDESK-7441

Fix superdesk-core dependency

Fix linter issues

SDESK-7441
@eos87 eos87 requested a review from MarkLark86 November 27, 2024 21:31
@eos87 eos87 added the async label Nov 27, 2024
@eos87 eos87 added this to the 3.0 milestone Nov 27, 2024
server/planning/assignments/module.py Outdated Show resolved Hide resolved
server/planning/assignments/module.py Outdated Show resolved Hide resolved
server/planning/assignments/service.py Outdated Show resolved Hide resolved
server/planning/events/module.py Outdated Show resolved Hide resolved
server/planning/planning/module.py Outdated Show resolved Hide resolved
server/planning/types/planning.py Outdated Show resolved Hide resolved
server/planning/types/planning.py Outdated Show resolved Hide resolved
server/planning/types/planning.py Outdated Show resolved Hide resolved
server/planning/types/planning.py Outdated Show resolved Hide resolved
server/planning/types/planning.py Outdated Show resolved Hide resolved
@eos87 eos87 requested a review from BrianMwangi21 November 28, 2024 17:22
@eos87
Copy link
Author

eos87 commented Nov 28, 2024

Hey @MarkLark86, I've made the changes and left a couple of comments in the "unresolved" conversations. I added this small helper method

@classmethod
def to_elastic_properties(cls) -> dict[Literal["properties"], Any]:
"""Generates the elastic mapping properties for the current dataclass"""
json_schema = TypeAdapter(cls).json_schema()
return json_schema_to_elastic_mapping(json_schema)
do you think this could be useful in other places? At the moment I'm using it like this
"assigned_to": {
"type": "object",
"properties": CoverageAssignedTo.to_elastic_properties(),
},
to avoid duplicating the same thing just in the form of a elastic mapping. I guess there would be other places where we would need something similar.

@MarkLark86
Copy link
Collaborator

Hey @MarkLark86, I've made the changes and left a couple of comments in the "unresolved" conversations. I added this small helper method

@classmethod
def to_elastic_properties(cls) -> dict[Literal["properties"], Any]:
"""Generates the elastic mapping properties for the current dataclass"""
json_schema = TypeAdapter(cls).json_schema()
return json_schema_to_elastic_mapping(json_schema)

do you think this could be useful in other places? At the moment I'm using it like this

"assigned_to": {
"type": "object",
"properties": CoverageAssignedTo.to_elastic_properties(),
},

to avoid duplicating the same thing just in the form of a elastic mapping. I guess there would be other places where we would need something similar.

Not sure there would be many other places where this would be used, as it's mostly covered by the new type system. This use case is an edge case, all others I can think of are for the storage and search of the data itself, so should be fine to leave where it is

@BrianMwangi21
Copy link

Hey @eos87 just a few more changes to some fields from mandatory to optional:

For EventResourceModel the following fields:

  • name
  • event_contact_info
  • translations

For PlanningResourceModel the following fields:

  • coverages.coverage_id
  • coverages.assigned_to

@eos87
Copy link
Author

eos87 commented Nov 29, 2024

@BrianMwangi21 fixed event_contact_info and translations providing default factory constructors for them. As for name I think it should be required. I suppose we should not create an event without a name at least.

Regarding PlanningResourceModel, coverage_id has been fixed. assigned_to had already a default factory constructor.

@BrianMwangi21
Copy link

Thanks. I'll see what to do about the names.

eos87 added 2 commits December 2, 2024 11:46
SDESK-7441
@eos87 eos87 merged commit a4f7d42 into async Dec 3, 2024
10 of 20 checks passed
@eos87 eos87 deleted the hg/SDESK-7441-base-async-services-resources branch December 3, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants