-
Notifications
You must be signed in to change notification settings - Fork 16
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
#6464 #6465 add frontend plugin table and improve plugin data #6466
#6464 #6465 add frontend plugin table and improve plugin data #6466
Conversation
@@ -12,8 +11,7 @@ use service::auth::{Resource, ResourceAccessRequest}; | |||
|
|||
#[derive(Union)] | |||
pub enum PluginDataResponse { | |||
Response(PluginDataNode), | |||
Error(NodeError), | |||
Response(PluginDataConnector), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making this a connector, since for column plugins we want to query for data once
#[derive(Clone, PartialEq, Eq, Debug, Default, Serialize, Deserialize)] | ||
pub struct FrontendPluginFiles(pub Vec<FrontendPluginFile>); | ||
|
||
impl From<String> for FrontendPluginFiles { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sort of serialization was first introduced in backed plugins, it's quite convenient, can be made generic
#[derive(Clone, PartialEq, Eq, Debug, Default, Serialize, Deserialize)] | ||
pub struct FrontendPluginTypes(pub Vec<String>); | ||
|
||
impl From<String> for FrontendPluginTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only indicative for front end plugins, the type is a string rather then a enum, wanted to reduce/remove back end changes when new front end interface is defined
frontend_plugin (id) { | ||
id -> Text, | ||
code -> Text, | ||
entry_point -> Text, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first file to 'load' from files
sql!( | ||
connection, | ||
r#" | ||
DROP TABLE plugin_data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no data yet, can drop
@@ -102,7 +95,7 @@ impl PluginDataFilterInput { | |||
pub fn to_domain(self) -> PluginDataFilter { | |||
PluginDataFilter { | |||
id: self.id.map(EqualFilter::from), | |||
plugin_name: self.plugin_name.map(EqualFilter::from), | |||
plugin_code: self.plugin_name.map(EqualFilter::from), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there meant to be a difference between name and code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think graphql sort/filter/inputs needs to expose code instead of name too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done in my PR #6548
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, approving this one but would like to see name and code cleaned up in downstream PR 🙏
|
||
pub fn find_one_by_id(&self, id: &str) -> Result<Option<FrontendPluginRow>, RepositoryError> { | ||
let result = frontend_plugin_dsl::frontend_plugin | ||
.filter(frontend_plugin_dsl::id.eq(id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit... can we fix this one not to use the _dsl alias?
@@ -102,7 +95,7 @@ impl PluginDataFilterInput { | |||
pub fn to_domain(self) -> PluginDataFilter { | |||
PluginDataFilter { | |||
id: self.id.map(EqualFilter::from), | |||
plugin_name: self.plugin_name.map(EqualFilter::from), | |||
plugin_code: self.plugin_name.map(EqualFilter::from), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think graphql sort/filter/inputs needs to expose code instead of name too?
@@ -56,7 +56,7 @@ fn validate(ctx: &ServiceContext, input: &UpdatePluginData) -> Result<(), Update | |||
if input.related_record_type != plugin_data.related_record_type { | |||
return Err(UpdatePluginDataError::RelatedRecordTypeDoesNotMatch); | |||
} | |||
if input.plugin_name != plugin_data.plugin_name { | |||
if input.plugin_name != plugin_data.plugin_code { | |||
return Err(UpdatePluginDataError::PluginNameDoesNotMatch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be good to update error type here too..
@lache-melvin thanks for review i've updated error name and changed dsl here: 2f6beb4 |
Closing as no extra changes to stack branch: #6485 |
This PR is ready for review but
I haven't self reviewed it yet sorryor fixed unit tests (tests fixed downstream), downstream PR has all of the changes in the stacked PRs, with documentation at some test examples: #6485Fixes #6465
Fixes #6464
👩🏻💻 What does this PR do?
💌 Any notes for the reviewer?
The "files" field on front end plugin stores base64 for now, can store it as string though.
Do we need related record type ? Would it be better to have 'plugin reference' ? There will be follow up issue about plugin data (like having central plugin data, making sure it sync, using plugin data as setting for plugins, etc)