[web] Turn `ThreadSettingsModal` into functional component
ClosedPublic
Actions

Authored by atul on Apr 24 2022, 12:54 PM.

Details

Reviewers

• jacek
tomek

Commits

rCOMMc952321494b1: [web] Turn `ThreadSettingsModal` into functional component

Summary

This diff completely removes the remnants of the ThreadSettingsModal class component. Specifically, this diff lifts the render() function of the class component to the return value of the outer functional component.

Now that we've turned ThreadSettingsModal into a functional component, we're going to continue the refactor by pushing "tab-specific" state/functionality to the ThreadSettings[General/Privacy/Delete]Tab child components (which are also now functional components)

Depends on D3822

Test Plan

Open ThreadSettingsModal
Click around, make some changes, hit save
Make sure that everything continues to look/behave as expected

Diff Detail

Repository

rCOMM Comm

Branch

land-april26 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 24 2022, 12:54 PM

Herald added subscribers: • abosh, • karol, • adrian, ashoat. · View Herald TranscriptApr 24 2022, 12:54 PM

(As of this diff, we've trimmed ThreadSettingsModal from being ~850 lines before refactor => 409 lines... will be even smaller after pushing down state/functionality to the child "tab" components)

atul retitled this revision from [web] Turn `ThreadSettingsModal` into ` functional component to [web] Turn `ThreadSettingsModal` into functional component.Apr 24 2022, 12:57 PM

Harbormaster completed remote builds in B8485: Diff 11830.Apr 24 2022, 12:59 PM

atul requested review of this revision.Apr 24 2022, 12:59 PM

atul added a child revision: D3826: [web] Remove duplicate `thread[Name/Description]Disabled` props in `ThreadSettingsGeneralTab` component.Apr 24 2022, 1:05 PM

In D3825#106086, @atul wrote:

(As of this diff, we've trimmed ThreadSettingsModal from being ~850 lines before refactor => 409 lines... will be even smaller after pushing down state/functionality to the child "tab" components)

Nice!

This revision is now accepted and ready to land.Apr 26 2022, 8:09 AM

atul mentioned this in D3823: [web] Lift `buttons: React.Node` from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.Apr 26 2022, 10:18 AM

rebase

Harbormaster completed remote builds in B8566: Diff 11945.Apr 26 2022, 10:28 AM

atul edited the summary of this revision. (Show Details)Apr 26 2022, 12:25 PM

atul added a parent revision: D3822: [web] Lift `mainContent: React.Node` from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.

rebase

Harbormaster completed remote builds in B8583: Diff 11964.Apr 26 2022, 1:07 PM

rebase before landing

Harbormaster completed remote builds in B8668: Diff 12068.Apr 28 2022, 1:16 PM

Closed by commit rCOMMc952321494b1: [web] Turn `ThreadSettingsModal` into functional component (authored by atul). · Explain WhyApr 28 2022, 1:18 PM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMc952321494b1: [web] Turn `ThreadSettingsModal` into functional component.

Revision Contents
Changeset List

Path

Size

web/

modals/

threads/

thread-settings-modal.react.js

124 lines

Diff 11964

View Options

web/modals/threads/thread-settings-modal.react.js

use actix_web::FromRequest;		use actix_web::FromRequest;
use chrono::Utc;		use chrono::Utc;
use comm_services_lib::{		use comm_services_lib::{
auth::UserIdentity,		auth::{AuthService, AuthorizationCredential},
blob::client::{BlobServiceClient, BlobServiceError},		blob::client::{BlobServiceClient, BlobServiceError},
crypto::aes256,		crypto::aes256,
database,		database,
};		};
use derive_more::{Display, Error, From};		use derive_more::{Display, Error, From};
use std::{		use std::{
collections::HashMap,		collections::HashMap,
future::{ready, Ready},		future::{ready, Future},
		pin::Pin,
sync::Arc,		sync::Arc,
};		};
use tracing::{error, trace};		use tracing::{error, trace, warn};

use crate::{		use crate::{
config::CONFIG,		config::CONFIG,
database::{		database::{
client::{DatabaseClient, ReportsPage},		client::{DatabaseClient, ReportsPage},
item::{ReportContent, ReportItem},		item::{ReportContent, ReportItem},
},		},
email::{config::EmailConfig, ReportEmail},		email::{config::EmailConfig, ReportEmail},
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines	) -> Self {
Self {		Self {
db,		db,
blob_client,		blob_client,
requesting_user_id: None,		requesting_user_id: None,
email_config: email_config.map(Arc::new),		email_config: email_config.map(Arc::new),
}		}
}		}

pub fn authenticated(&self, user: UserIdentity) -> Self {		/// Clones the service with a new auth identity. When the credential is
let user_id = user.user_id.to_string();		/// a service-to-service, the `user_id` is None.
		pub fn with_authentication(&self, token: AuthorizationCredential) -> Self {
		let requesting_user_id = match &token {
		AuthorizationCredential::ServicesToken(_) => None,
		AuthorizationCredential::UserToken(user) => {
		Some(user.user_id.to_string())
		}
		};
Self {		Self {
db: self.db.clone(),		db: self.db.clone(),
email_config: self.email_config.clone(),		email_config: self.email_config.clone(),
blob_client: self.blob_client.with_user_identity(user),		blob_client: self.blob_client.with_authentication(token),
requesting_user_id: Some(user_id),		requesting_user_id,
}		}
}		}

pub async fn save_reports(		pub async fn save_reports(
&self,		&self,
inputs: Vec<ReportInput>,		inputs: Vec<ReportInput>,
) -> ServiceResult<Vec<ReportID>> {		) -> ServiceResult<Vec<ReportID>> {
let mut reports = Vec::with_capacity(inputs.len());		let mut reports = Vec::with_capacity(inputs.len());
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Lines	impl ReportsService {
) -> ServiceResult<ReportsPage> {		) -> ServiceResult<ReportsPage> {
let page = self.db.scan_reports(cursor, page_size).await?;		let page = self.db.scan_reports(cursor, page_size).await?;
Ok(page)		Ok(page)
}		}
}		}

impl FromRequest for ReportsService {		impl FromRequest for ReportsService {
type Error = actix_web::Error;		type Error = actix_web::Error;
type Future = Ready<Result<Self, actix_web::Error>>;		type Future = Pin<Box<dyn Future<Output = Result<Self, actix_web::Error>>>>;

#[inline]		#[inline]
fn from_request(		fn from_request(
req: &actix_web::HttpRequest,		req: &actix_web::HttpRequest,
_payload: &mut actix_web::dev::Payload,		_payload: &mut actix_web::dev::Payload,
) -> Self::Future {		) -> Self::Future {
		use actix_web::error::{ErrorForbidden, ErrorInternalServerError};
use actix_web::HttpMessage;		use actix_web::HttpMessage;

let Some(service) = req.app_data::<ReportsService>() else {		let base_service =
		req.app_data::<ReportsService>().cloned().ok_or_else(\|\| {
tracing::error!(		tracing::error!(
"FATAL! Failed to extract ReportsService from actix app_data. \		"FATAL! Failed to extract ReportsService from actix app_data. \
Check HTTP server configuration"		Check HTTP server configuration"
);		);
return ready(Err(actix_web::error::ErrorInternalServerError("Internal server error")));		ErrorInternalServerError("Internal server error")
};		});

let auth_service =		let auth_service =
if let Some(user_identity) = req.extensions().get::<UserIdentity>() {		req.app_data::<AuthService>().cloned().ok_or_else(\|\| {
tracing::trace!("Found user identity. Creating authenticated service");		tracing::error!(
service.authenticated(user_identity.clone())		"FATAL! Failed to extract AuthService from actix app_data. \
} else {		Check HTTP server configuration"
tracing::trace!(
"No user identity found. Leaving unauthenticated service"
);		);
service.clone()		ErrorInternalServerError("Internal server error")
};		});

		let request_auth_value =
		req.extensions().get::<AuthorizationCredential>().cloned();

ready(Ok(auth_service))		Box::pin(async move {
		let auth_service = auth_service?;
		let base_service = base_service?;

		// This is Some for endpoints hidden behind auth validation middleware
		let auth_token = match request_auth_value {
		Some(token @ AuthorizationCredential::UserToken(_)) => token,
		Some(_) => {
		// Reports service shouldn't be called by other services
		warn!("Reports service requires user authorization");
		return Err(ErrorForbidden("Forbidden"));
		}
		None => {
		// Unauthenticated requests get a service-to-service token
		let services_token =
		auth_service.get_services_token().await.map_err(\|err\| {
		error!("Failed to get services token: {err}");
		ErrorInternalServerError("Internal server error")
		})?;
		AuthorizationCredential::ServicesToken(services_token)
		}
		};
		let service = base_service.with_authentication(auth_token);
		Ok(service)
		})
		michalUnsubmitted Not Done Inline Actions Do you think it would make sense to move this into authorization middleware? So you can specify something like .wrap(get_comm_authentication_middleware(AllowedAuth::user())) // or .wrap(get_comm_authentication_middleware(AllowedAuth::unauthorized())) // -> this would insert `AuthorizationCredential` with service token automatically // or .wrap(get_comm_authentication_middleware(AllowedAuth::services().user())) I don't remember exactly what the API for the reports service is, but right now this diff would allow everyone to "authenticate" to the reports service by just not providing credentials (and get the service token by default). This should probably be scoped per (group of) endpoints. michal: Do you think it would make sense to move this into authorization middleware? So you can specify…
		bartekAuthorUnsubmitted Done Inline Actions I played with various solutions a bit (spent a little too long on this). Your suggestion makes sense in case of API, and looks like it is doable. However, regarding having protected and unprotected endpoints: right now this diff would allow everyone to "authenticate" to the reports service by just not providing credentials (and get the service token by default). This should probably be scoped per (group of) endpoints. This is expected, we don't authenticate endpoints yet because we don't have client token. However, this code works well with your existing auth middleware - endpoints wrapped with it will return 401 even before this code is executed. so in the future, when we have client token, doing the following will work: let auth_middleware = get_comm_authentication_middleware(); // this will require client token and fail if not provided web::scope("/authenticated") .wrap(auth_middleware) .service(...) // this will accept both authenticated and anonymous requests. // for the latter, service-to-service token will be obtained web::scope("/anonymous") // .wrap(auth_middleware) // no wrap .service(...) To conclude, I might refactor to look more as you suggested, but it makes more sense to do it later when we have client token available (2nd part of my task that is currently blocked). I can create a follow up task for this if you wish bartek: I played with various solutions a bit (spent a little too long on this). Your suggestion makes…
		michalUnsubmitted Not Done Inline Actions Makes sense, I got confused a bit michal: Makes sense, I got confused a bit
}		}
}		}

struct ProcessedReport {		struct ProcessedReport {
id: ReportID,		id: ReportID,
db_item: ReportItem,		db_item: ReportItem,
email: ReportEmail,		email: ReportEmail,
}		}
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines

[web] Turn `ThreadSettingsModal` into functional componentClosedPublicActions