Skip to content
This repository has been archived by the owner on Nov 21, 2024. It is now read-only.

author decisions: decision logic added #76

Merged
merged 1 commit into from
Aug 22, 2024
Merged

author decisions: decision logic added #76

merged 1 commit into from
Aug 22, 2024

Conversation

DonHaul
Copy link
Contributor

@DonHaul DonHaul commented Aug 13, 2024 •
edited
Loading

Besides Implementing the decision logic. I propose also two main changes in how we interact with airflow.

  1. Use shared tasks, for now if two dags use the same task we were simply copy pasting.
  2. Use base backoffice hook, instead of creating brand new one (else for each different model we have, a hook is created for it ) - to consider also deprecating the workflowticket_management_hook

@DonHaul
Copy link
Contributor Author

DonHaul commented Aug 13, 2024

Fixes cern-sis/issues-inspire#522

@DonHaul DonHaul force-pushed the decision-action-log branch 4 times, most recently from 2afcfd9 to 0cb110a Compare August 15, 2024 11:52
This was linked to issues Aug 15, 2024
Update Resolution options for authors cern-sis/issues-inspire#522
Closed
Decision action log cern-sis/issues-inspire#518
Closed
@DonHaul DonHaul force-pushed the decision-action-log branch 2 times, most recently from 19d3644 to c288e17 Compare August 19, 2024 15:19
@DonHaul DonHaul marked this pull request as ready for review August 20, 2024 08:13
@DonHaul DonHaul requested a review from drjova August 20, 2024 08:13
@DonHaul DonHaul force-pushed the decision-action-log branch from ec0e647 to d65c2fe Compare August 20, 2024 08:19
@DonHaul
Copy link
Contributor Author

DonHaul commented Aug 20, 2024

Tests will fail as the connection.json were not being imported to the github workflows

drjova
drjova suggested changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@drjova drjova left a comment

Choose a reason for hiding this comment

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

Also, we since we have the actions for accept, reject, accept_curate, we can add the decision there. It's ok to keep the endpoint too

backoffice/backoffice/workflows/admin.py Outdated
@@ -30,8 +30,41 @@ def has_permission(self, request):
)


class ModelAdmin(admin.ModelAdmin):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename it, not to have the same name as the admin module

backoffice/backoffice/workflows/api/views.py Outdated
workflow_id = request.data.get("workflow_id")
action = request.data.get("action")

if not all([workflow_id, action]):
Copy link
Contributor

Choose a reason for hiding this comment

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

That's why we have the serializer :) we can check if it's valid and then save

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewing. Same approach was also implemented in the WorkflowTicketViewSet should be reviewed as well
https://github.com/inspirehep/backoffice/blob/main/backoffice/backoffice/workflows/api/views.py#L84
Ticket created cern-sis/issues-inspire#544

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for creating the issue, on django it's a best practise to use the serializer for validation

backoffice/backoffice/workflows/constants.py Outdated
accept_curate = "accept_curate", "author_create_approved_dag"


DECISION_CHOICES = ResolutionDags.choices + []
Copy link
Contributor

Choose a reason for hiding this comment

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

why + []?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was so I would remember to add the decisions regarding other stuff in the future. Remove

@DonHaul DonHaul force-pushed the decision-action-log branch 4 times, most recently from 60d5d81 to 641708c Compare August 20, 2024 14:06
drjova
drjova reviewed Aug 20, 2024
View reviewed changes
.github/workflows/test-workflows.yml Outdated
@@ -51,6 +51,7 @@ jobs:
-v "$(pwd)"/tests:/opt/airflow/tests
-v "$(pwd)"/requirements-test.txt:/opt/airflow/requirements-test.txt
-v "$(pwd)"/data:/opt/airflow/data
-v "$(pwd)"/scripts/connections/connections.json:/opt/airflow/connections.json
Copy link
Contributor

Choose a reason for hiding this comment

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

at the end we can mount all the scripts directory :)

backoffice/backoffice/workflows/api/views.py Outdated

logger = logging.getLogger(__name__)


def add_decision(workflow_id, user, action):
Copy link
Contributor

Choose a reason for hiding this comment

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

let's have a quick discussion about that :)

@DonHaul DonHaul force-pushed the decision-action-log branch 5 times, most recently from dc2d3fd to 99a2fd5 Compare August 20, 2024 15:05
@DonHaul DonHaul mentioned this pull request Aug 21, 2024
Merged
@DonHaul DonHaul force-pushed the decision-action-log branch from 99a2fd5 to 4a4c9f1 Compare August 21, 2024 11:15
@DonHaul DonHaul mentioned this pull request Aug 21, 2024
Closed
@DonHaul DonHaul force-pushed the decision-action-log branch 3 times, most recently from 4278bf5 to 067e814 Compare August 22, 2024 11:31
@DonHaul DonHaul force-pushed the decision-action-log branch from 067e814 to 0494814 Compare August 22, 2024 11:36
@drjova
Copy link
Contributor

drjova commented Aug 22, 2024

@DonHaul also here could you please fix the commit message? :) Is too long and we should be consistent with https://confluence.cern.ch/display/RCSSIS/Contributing+Guidelines

@DonHaul DonHaul force-pushed the decision-action-log branch from 0494814 to 96b660d Compare August 22, 2024 12:08
@DonHaul
workflows: add decision table and populate it
f658c3d 
* Decision is now added on the /resolve api

* ref: cern-sis/issues-inspire/issues/522
@DonHaul DonHaul force-pushed the decision-action-log branch from 96b660d to f658c3d Compare August 22, 2024 12:17
@DonHaul DonHaul merged commit 6a64407 into inspirehep:main Aug 22, 2024
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@drjova drjova drjova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update Resolution options for authors Decision action log
2 participants
@DonHaul @drjova