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

Update OONIRun spec #278

Closed
wants to merge 2 commits into from
Closed

Update OONIRun spec #278

wants to merge 2 commits into from

Conversation

FedericoCeratto
Copy link

Updated based on the current implementation.
ooni/backend#678
ooni/backend#699
ooni/backend#702

@FedericoCeratto FedericoCeratto marked this pull request as ready for review October 10, 2023 19:33

"description": "",
"oonirun_id": "", // `string` OONI Run link identifier.
Copy link
Member

Choose a reason for hiding this comment

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

For consistency can we do a s/oonirun_id/ooni_run_link_id in the backend and frontend?

Copy link
Member

Choose a reason for hiding this comment

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

Why are we passing the ooni_run_link_id as part of the create operation?

Copy link
Author

@FedericoCeratto FedericoCeratto Oct 27, 2023

Choose a reason for hiding this comment

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

The oonirun_id variable has been renamed in ooni/backend@a376270 and related commits.

When support for translations + URL updates have been added to the OONI Run links we implemented them as appending new records to the database table in order:

  • to keep history of previous versions in case users want to fetch an older version instead of the latest one
  • to allow incremental improvements of translations
    This is implemented in Add translation support for OONI Run v2 backend#699 - also see https://docs.google.com/document/d/1tsik0DZLOgYHh01RsLWWXp3ZHhHB_C07j8dcaMbGB9M/edit
    On the database side rather than updating an existing OONI Run we are creating either a new version or a new translations, hence every action can be done with the /api/_/ooni_run/create path.
    To add more items related to an already existing OONI Run link the Web UI passes ooni_run_link_id .
    When absent the API generates a new ooni_run_link_id value.


}
```

## 4.2 UPDATE an existing OONI Run link
Copy link
Member

Choose a reason for hiding this comment

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

What happened to updates for OONI Run links? I believe it is supported, so why was the update text removed?

Copy link
Author

Choose a reason for hiding this comment

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

See previous reply.

@@ -344,14 +358,20 @@ authentication should be handled.
When you `CREATE` a new OONI RUN link, the client sends a HTTP `POST`
request conforming to the following:

`POST /api/v1/ooni_run`
`POST /api/_/ooni_run/create` with optional `id` argument
Copy link
Member

Choose a reason for hiding this comment

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

What does this "with optional id argument" mean? Where is this argument passed and what does it do?

Copy link
Author

Choose a reason for hiding this comment

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

See previous reply.

- "only_latest": <boolean> // Return only the latest version of each OONI Run link
- "include_archived": <boolean> // Include archived links
- "only_mine": <boolean> // Show only links owned by the current user
- "ids": <string> // Comma-separated OONI Run link ID numbers to filter for
Copy link
Member

Choose a reason for hiding this comment

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

How are these "arguments" passed? Does this mean these are URL parameters? If that is the case, then we should clarify that's what we mean specify how they are passed.

Copy link
Author

Choose a reason for hiding this comment

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

They are described in https://api.ooni.io/apidocs/#/default/get_api___ooni_run_list - I'll update the PR

Copy link
Member

@hellais hellais left a comment

Choose a reason for hiding this comment

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

Some changes need to be done for clarity. The UPDATE method sections is missing. We should, for consistency, change the name of oonirun_id to ooni_run_link_id so it's consistent with what we use elsewhere.


"v": 1 // Response format version
Copy link
Member

Choose a reason for hiding this comment

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

Since this appears in many parts of the spec, we probably should make a table at the beginning of the doc explaining this v option and include a list of current version (which now starts from 1)

Copy link
Author

@FedericoCeratto FedericoCeratto Oct 26, 2023

Choose a reason for hiding this comment

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

Added a paragraph explaining it.

@hellais
Copy link
Member

hellais commented Feb 23, 2024

This is superseeded by #292

@hellais hellais closed this Feb 23, 2024
@hellais hellais deleted the ooni-run-v2-update branch February 23, 2024 15:06
@hellais hellais restored the ooni-run-v2-update branch February 23, 2024 15:06
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