Add updateTrustedDomain GraphQL API
#276
Conversation
|
The pr below should be merged first. |
src/graphql.rs
Outdated
| @@ -522,6 +522,9 @@ struct MockAgentManager {} | |||
| #[cfg(test)] | |||
| #[async_trait::async_trait] | |||
| impl AgentManager for MockAgentManager { | |||
| async fn broadcast_trusted_domains(&self) -> Result<(), anyhow::Error> { | |||
| Ok(()) | |||
There was a problem hiding this comment.
The broadcast_trusted_domains function in MockAgentManager has been modified to return Ok(()) for the purpose of executing test code
henry0715-dev
changed the title
updateTrustedDomain GraphQL APIupdateTrustedDomain GraphQL API
henry0715-dev
changed the title
updateTrustedDomain GraphQL APIupdateTrustedDomain GraphQL API
|
Could you rebase please? |
henry0715-dev
force-pushed
the
henry/modify-trusted-domains
branch
from
428be1a to
63f58fa
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
==========================================
- Coverage 70.26% 70.19% -0.07%
==========================================
Files 67 67
Lines 14400 14831 +431
==========================================
+ Hits 10118 10411 +293
- Misses 4282 4420 +138 ☔ View full report in Codecov by Sentry. |
henry0715-dev
changed the title
updateTrustedDomain GraphQL APIupdateTrustedDomain GraphQL API
henry0715-dev
force-pushed
the
henry/modify-trusted-domains
branch
from
63f58fa to
e15d2ae
Compare
henry0715-dev
changed the title
updateTrustedDomain GraphQL APIupdateTrustedDomain GraphQL API
henry0715-dev
force-pushed
the
henry/modify-trusted-domains
branch
from
e15d2ae to
15add15
Compare
It's done. |
|
@sophie-cluml |
henry0715-dev
force-pushed
the
henry/modify-trusted-domains
branch
2 times, most recently
from
3011f55 to
4d2108c
Compare
src/graphql/trusted_domain.rs
Outdated
| @@ -62,6 +62,29 @@ impl TrustedDomainMutation { | |||
| Ok(name) | |||
| } | |||
|
|
|||
| /// Update a trusted domain, returning the new value if it passes domain validation. | |||
There was a problem hiding this comment.
Since this API does not run validation, I think the documentation comment needs to be modified.
CHANGELOG.md
Outdated
| - Added the `updateTrustedDomain` GraphQL API to modify the list of trusted | ||
| domains. |
There was a problem hiding this comment.
Could you clarify the meaning of modify the list here please? My understanding of updateTrustedDomain GraphQL API is that the API allows updating a single trusted domain. So I think ... to modify a trusted domain. is rather straightforward. But it could be that you intended something else, so please let me know what you think.
henry0715-dev
force-pushed
the
henry/modify-trusted-domains
branch
from
4d2108c to
9b07b68
Compare
|
The clippy error that occurred during the rust version update was corrected by another PR(#364). |
Cargo.toml
Outdated
| @@ -43,6 +43,7 @@ tokio = "1" | |||
| tower-http = { version = "0.6", features = ["fs", "trace"] } | |||
| tracing = "0.1" | |||
| vinum = { git = "https://github.com/vinesystems/vinum.git", tag = "1.0.3" } | |||
| regex = "1" | |||
There was a problem hiding this comment.
I don't think this is no longer needed.
There was a problem hiding this comment.
I finished editing it.
henry0715-dev
force-pushed
the
henry/modify-trusted-domains
branch
from
9b07b68 to
8531245
Compare
| let name = { | ||
| let store = crate::graphql::get_store(ctx).await?; | ||
| let map = store.trusted_domain_map(); | ||
| let old = review_database::TrustedDomain::from(old); | ||
| let new = review_database::TrustedDomain::from(new); | ||
| map.update(&old, &new)?; | ||
| new.name | ||
| }; |
There was a problem hiding this comment.
It seems that your style, where a variable is declared early and assigned a specific value later, is still being used. I believe this approach is not ideal. Moreover, in this context, the name variable doesn't appear to be essential.
There was a problem hiding this comment.
Do you have any specific expectations for the return type of the GraphQL API?
It seems that Result<()> cannot be used.
42 | #[Object]
| ^^^^^^^^^ the trait `OutputType` is not implemented for `()`
Are you considering declaring and using a custom scalar type like Void?
There was a problem hiding this comment.
I apologize for my confusion. I thought this was similar to what you had done before, but it seems inevitable for the later asynchronous code. Once again, I’m sorry for my mistake in not catching the later part.
CHANGELOG.md
Outdated
| @@ -7,6 +7,11 @@ Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Added the `updateTrustedDomain` GraphQL API, allowing you to modify a trusted | |||
There was a problem hiding this comment.
I think "users" sound better than just "you".
There was a problem hiding this comment.
I finished editing it.
Thank you.
src/graphql/trusted_domain.rs
Outdated
| @@ -63,6 +63,29 @@ impl TrustedDomainMutation { | |||
| Ok(name) | |||
| } | |||
|
|
|||
| /// Update a trusted domain, returning the new value. | |||
There was a problem hiding this comment.
Rather than the imperative form, the third-person singular form of the verb is preferred.
There was a problem hiding this comment.
I finished editing it.
Thank you.
henry0715-dev
force-pushed
the
henry/modify-trusted-domains
branch
from
8531245 to
f6f98f9
Compare
Closes #2