1
0
Fork 0
mirror of https://gitlab.com/bramw/baserow.git synced 2025-04-07 22:35:36 +00:00

Resolve "Removing page parameters corrupts any workflow_action enabled element that had that parameter set"

This commit is contained in:
Afonso Silva 2025-01-06 14:16:11 +00:00 committed by Peter Evans
parent 01e664073e
commit 847a7f2571
18 changed files with 359 additions and 73 deletions

View file

@ -0,0 +1,7 @@
{
"type": "bug",
"message": "[Builder] Fix worfklow action navigation for pages with no parameters",
"issue_number": 2833,
"bullet_points": [],
"created_at": "2024-11-26"
}

View file

@ -234,11 +234,6 @@ export default {
this.element.parent_element_id
)
},
parentElementType() {
return this.parentElement
? this.$registry.get('element', this.parentElement?.type)
: null
},
inError() {
return this.elementType.isInError({
page: this.elementPage,

View file

@ -153,6 +153,7 @@ export default {
watch: {
'destinationPage.path_params': {
handler(value) {
this.updatePageParameters()
this.refreshParametersInError()
},
deep: true,
@ -191,6 +192,7 @@ export default {
}
this.navigateTo = navigateTo
}
this.updatePageParameters()
this.refreshParametersInError()
},
methods: {

View file

@ -4,12 +4,13 @@
<Tab
v-for="pageSidePanelType in pageSidePanelTypes"
:key="pageSidePanelType.getType()"
:tooltip="
element && pageSidePanelType.isDeactivated(element)
? pageSidePanelType.getDeactivatedText()
:tooltip="getTooltipMessage(pageSidePanelType)"
:title="pageSidePanelType.label"
:append-icon="
pageSidePanelType.isInError(sidePanelContext)
? 'page-editor__side-panel--error iconoir-warning-circle'
: null
"
:title="pageSidePanelType.label"
:disabled="!element || pageSidePanelType.isDeactivated(element)"
>
<ReadOnlyForm
@ -40,7 +41,7 @@ import EmptySidePanelState from '@baserow/modules/builder/components/page/sidePa
export default {
name: 'PageSidePanels',
components: { EmptySidePanelState },
inject: ['workspace'],
inject: ['workspace', 'builder'],
computed: {
...mapGetters({
element: 'element/getSelected',
@ -48,6 +49,24 @@ export default {
pageSidePanelTypes() {
return this.$registry.getOrderedList('pageSidePanel')
},
sidePanelContext() {
const page = this.$store.getters['page/getSelected']
return {
page,
builder: this.builder,
element: this.element,
}
},
},
methods: {
getTooltipMessage(pageSidePanelType) {
if (this.element && pageSidePanelType.isDeactivated(this.element)) {
return pageSidePanelType.getDeactivatedText()
} else if (pageSidePanelType.isInError(this.sidePanelContext)) {
return pageSidePanelType.getInErrorText()
}
return null
},
},
}
</script>

View file

@ -35,13 +35,18 @@ export const ContainerElementTypeMixin = (Base) =>
* A Container element without any child elements is invalid. Return true
* if there are no children, otherwise return false.
*/
isInError({ page, element }) {
isInError({ page, element, builder }) {
const children = this.app.store.getters['element/getChildren'](
page,
element
)
return !children.length
// A container element needs at least one child.
if (!children.length) {
return true
}
return super.isInError({ page, element, builder })
}
}

View file

@ -209,12 +209,37 @@ export class ElementType extends Registerable {
return true
}
/**
* If this element supports events, then this function will iterate over
* them and ensure that they are configured properly. If they are in error,
* this will propagate into an 'element in error' state.
*/
workflowActionsInError({ page, element, builder }) {
const workflowActions = this.app.store.getters[
'workflowAction/getElementWorkflowActions'
](page, element.id)
return workflowActions.some((workflowAction) => {
const workflowActionType = this.app.$registry.get(
'workflowAction',
workflowAction.type
)
return workflowActionType.isInError(workflowAction, {
page,
element,
builder,
})
})
}
/**
* Returns whether the element configuration is valid or not.
* @param {object} param An object containing the page, element, and builder
* @returns true if the element is in error
*/
isInError({ page, element, builder }) {
if (this.getEvents(element).length > 0) {
return this.workflowActionsInError({ page, element, builder })
}
return false
}
@ -735,7 +760,7 @@ export class FormContainerElementType extends ContainerElementTypeMixin(
* A form container is invalid if it has no workflow actions, or it has no
* children.
*/
isInError({ page, element }) {
isInError({ page, element, builder }) {
const workflowActions = this.app.store.getters[
'workflowAction/getElementWorkflowActions'
](page, element.id)
@ -744,7 +769,7 @@ export class FormContainerElementType extends ContainerElementTypeMixin(
return true
}
return super.isInError({ page, element })
return super.isInError({ page, element, builder })
}
}
@ -1088,8 +1113,11 @@ export class HeadingElementType extends ElementType {
* A value is mandatory for the Heading element. Return true if the value
* is empty to indicate an error, otherwise return false.
*/
isInError({ element }) {
return element.value.length === 0
isInError({ page, element, builder }) {
if (element.value.length === 0) {
return true
}
return super.isInError({ page, element, builder })
}
getDisplayName(element, applicationContext) {
@ -1132,8 +1160,11 @@ export class TextElementType extends ElementType {
* A value is mandatory for the Text element. Return true if the value
* is empty to indicate an error, otherwise return false.
*/
isInError({ element }) {
return element.value.length === 0
isInError({ page, element, builder }) {
if (element.value.length === 0) {
return true
}
return super.isInError({ page, element, builder })
}
getDisplayName(element, applicationContext) {
@ -1182,7 +1213,7 @@ export class LinkElementType extends ElementType {
* When the Navigate To is a Custom URL, a Destination URL value must be
* provided.
*/
isInError({ element, builder }) {
isInError({ page, element, builder }) {
// A Link without any text isn't usable
if (!element.value) {
return true
@ -1199,7 +1230,7 @@ export class LinkElementType extends ElementType {
} else if (element.navigation_type === 'custom') {
return Boolean(!element.navigate_to_url)
}
return true
return super.isInError({ page, element, builder })
}
getDisplayName(element, applicationContext) {
@ -1267,12 +1298,19 @@ export class ImageElementType extends ElementType {
* to indicate an error when an image source doesn't exist, otherwise
* return false.
*/
isInError({ element }) {
if (element.image_source_type === IMAGE_SOURCE_TYPES.UPLOAD) {
return Boolean(!element.image_file?.url)
} else {
return Boolean(!element.image_url)
isInError({ page, element, builder }) {
if (
element.image_source_type === IMAGE_SOURCE_TYPES.UPLOAD &&
!element.image_file?.url
) {
return true
} else if (
element.image_source_type === IMAGE_SOURCE_TYPES.URL &&
!element.image_url
) {
return true
}
return super.isInError({ page, element, builder })
}
getDisplayName(element, applicationContext) {
@ -1319,7 +1357,7 @@ export class ButtonElementType extends ElementType {
* A Button element must have a Workflow Action to be considered valid. Return
* true if there are no Workflow Actions, otherwise return false.
*/
isInError({ page, element }) {
isInError({ page, element, builder }) {
// If Button without any label should be considered invalid
if (!element.value) {
return true
@ -1329,7 +1367,10 @@ export class ButtonElementType extends ElementType {
'workflowAction/getElementWorkflowActions'
](page, element.id)
return !workflowActions.length
if (!workflowActions.length) {
return true
}
return super.isInError({ page, element, builder })
}
getDisplayName(element, applicationContext) {
@ -1455,16 +1496,17 @@ export class ChoiceElementType extends FormElementType {
return !(element.required && !validOption)
}
isInError({ element }) {
switch (element.option_type) {
case CHOICE_OPTION_TYPES.MANUAL:
return element.options.length === 0
case CHOICE_OPTION_TYPES.FORMULAS: {
return element.formula_value === ''
}
default:
isInError({ page, element, builder }) {
if (element.option_type === CHOICE_OPTION_TYPES.MANUAL) {
if (element.options.length === 0) {
return true
}
} else if (element.option_type === CHOICE_OPTION_TYPES.FORMULAS) {
if (element.formula_value === '') {
return true
}
}
return super.isInError({ page, element, builder })
}
getDataSchema(element) {
@ -1557,7 +1599,7 @@ export class IFrameElementType extends ElementType {
* source_type. If the value doesn't exist, return true to indicate an error,
* otherwise return false.
*/
isInError({ element }) {
isInError({ page, element, builder }) {
if (element.source_type === IFRAME_SOURCE_TYPES.URL && !element.url) {
return true
} else if (
@ -1565,9 +1607,8 @@ export class IFrameElementType extends ElementType {
!element.embed
) {
return true
} else {
return false
}
return super.isInError({ page, element, builder })
}
getDisplayName(element, applicationContext) {
@ -1668,8 +1709,11 @@ export class RecordSelectorElementType extends CollectionElementTypeMixin(
* @param {Object} element the element to check the error
* @returns
*/
isInError({ element }) {
return !element.data_source_id
isInError({ page, element, builder }) {
if (!element.data_source_id) {
return true
}
return super.isInError({ page, element, builder })
}
getDataSchema(element) {

View file

@ -148,7 +148,8 @@
"style": "Style",
"visibility": "Visibility",
"events": "Events",
"eventsTabDeactivatedNoEvents": "This element doesn't support any events"
"eventsTabDeactivatedNoEvents": "This element doesn't support any events",
"eventsTabInError": "One or more actions are misconfigured."
},
"emptySidePanelState": {
"message": "Click on one of the elements to see more details"

View file

@ -13,7 +13,21 @@ export class pageSidePanelType extends Registerable {
return null
}
getDeactivatedText(element) {
/**
* The message which appears in the tooltip when the page side panel
* detects that the tab should be deactivated.
* @returns {string}
*/
getDeactivatedText() {
return ''
}
/**
* The message which appears in the tooltip when the page side panel
* is misconfigured and in-error.
* @returns {string}
*/
getInErrorText() {
return ''
}
@ -24,6 +38,17 @@ export class pageSidePanelType extends Registerable {
getOrder() {
return this.order
}
/**
* Returns whether this side panel is in error, or not. Allows us to append
* an error icon after the panel title so that the page designer can be
* informed.
* @param applicationContext
* @returns {boolean}
*/
isInError(applicationContext) {
return false
}
}
export class GeneralPageSidePanelType extends pageSidePanelType {
@ -93,11 +118,26 @@ export class EventsPageSidePanelType extends pageSidePanelType {
return EventsSidePanel
}
getDeactivatedText(element) {
/**
* The message which appears in the tooltip when the events page side panel
* detects that the selected element doesn't support any events.
* @returns {string}
*/
getDeactivatedText() {
const { i18n } = this.app
return i18n.t('pageSidePanelType.eventsTabDeactivatedNoEvents')
}
/**
* The message which appears in the tooltip when the events page side panel
* is misconfigured, as one or more workflow actions are in-error.
* @returns {string}
*/
getInErrorText() {
const { i18n } = this.app
return i18n.t('pageSidePanelType.eventsTabInError')
}
isDeactivated(element) {
const elementType = this.app.$registry.get('element', element.type)
return elementType.getEvents(element).length === 0
@ -106,4 +146,32 @@ export class EventsPageSidePanelType extends pageSidePanelType {
getOrder() {
return 40
}
/**
* Returns whether this side panel is in error, or not. Allows us to append
* an error icon after the panel title so that the page designer can be
* informed.
* @returns {boolean}
*/
isInError({ page, element, builder }) {
if (!element) {
// If we don't have an element, then this element type
// doesn't support events, so it can't be in-error.
return false
}
const workflowActions = this.app.store.getters[
'workflowAction/getElementWorkflowActions'
](page, element.id)
return workflowActions.some((workflowAction) => {
const workflowActionType = this.app.$registry.get(
'workflowAction',
workflowAction.type
)
return workflowActionType.isInError(workflowAction, {
page,
element,
builder,
})
})
}
}

View file

@ -12,26 +12,29 @@ export function defaultValueForParameterType(type) {
}
/**
* Responsible for detecting if an element's path parameters have diverged
* from the destination page's path parameters. This can happen if an element
* Responsible for detecting if a navigable record's path parameters have diverged
* from the destination page's path parameters. This can happen if a record
* points to a page, and then the page's parameters are altered.
*
* @param {Object} element The element's properties we'll validate.
* @param {Object} pages Page of this application.
* @returns {Boolean} Whether this resolvedUrl is external.
* @param {Object} navigationObject - An `element` or `workflowAction` object
* which points to navigation data. In the case of an `element` this could be
* a button, and in the case of a `workflowAction` this could be an "open page"
* workflow action type.
* @param {Array} pages - An array of "visible" pages in the application.
* @returns {Boolean} Whether this navigable object has parameters in error.
*/
export function pathParametersInError(element, pages) {
export function pathParametersInError(navigationObject, pages) {
if (
element.navigation_type === 'page' &&
!isNaN(element.navigate_to_page_id)
navigationObject.navigation_type === 'page' &&
!isNaN(navigationObject.navigate_to_page_id)
) {
const destinationPage = pages.find(
({ id }) => id === element.navigate_to_page_id
({ id }) => id === navigationObject.navigate_to_page_id
)
if (destinationPage) {
const destinationPageParams = destinationPage.path_params || []
const pageParams = element.page_parameters || []
const pageParams = navigationObject.page_parameters || []
const destinationPageParamNames = destinationPageParams.map(
({ name }) => name

View file

@ -41,12 +41,14 @@ export default function resolveElementUrl(
const toPath = compile(page.path, { encode: encodeURIComponent })
const pageParams = Object.fromEntries(
element.page_parameters.map(({ name, value }) => [
name,
PAGE_PARAM_TYPE_VALIDATION_FUNCTIONS[paramTypeMap[name]](
resolveFormula(value)
),
])
element.page_parameters
.filter(({ name }) => name in paramTypeMap)
.map(({ name, value }) => [
name,
PAGE_PARAM_TYPE_VALIDATION_FUNCTIONS[paramTypeMap[name]](
resolveFormula(value)
),
])
)
resolvedUrl = toPath(pageParams)
}

View file

@ -9,6 +9,7 @@ import { DataProviderType } from '@baserow/modules/core/dataProviderTypes'
import resolveElementUrl from '@baserow/modules/builder/utils/urlResolution'
import { ensureString } from '@baserow/modules/core/utils/validator'
import DeleteRowWorkflowActionForm from '@baserow/modules/builder/components/workflowAction/DeleteRowWorkflowActionForm.vue'
import { pathParametersInError } from '@baserow/modules/builder/utils/params'
export class NotificationWorkflowActionType extends WorkflowActionType {
static getType() {
@ -48,6 +49,19 @@ export class OpenPageWorkflowActionType extends WorkflowActionType {
return this.app.i18n.t('workflowActionTypes.openPageLabel')
}
/**
* Returns whether the open page configuration is valid or not.
* @param {object} workflowAction - The workflow action to validate.
* @param {object} param An object containing application context data.
* @returns true if the open page action is in error
*/
isInError(workflowAction, { element, builder }) {
return pathParametersInError(
workflowAction,
this.app.store.getters['page/getVisiblePages'](builder)
)
}
execute({
workflowAction,
applicationContext: { builder, mode },

View file

@ -6,3 +6,7 @@
flex: 1;
border-left: 1px solid $color-neutral-200;
}
.page-editor__side-panel--error {
color: $color-error-300;
}

View file

@ -31,6 +31,11 @@ export default {
required: false,
default: null,
},
appendIcon: {
type: String,
required: false,
default: null,
},
},
data() {
return {

View file

@ -30,6 +30,7 @@
>
<i v-if="tab.icon" :class="tab.icon"></i>
{{ tab.title }}
<i v-if="tab.appendIcon" :class="tab.appendIcon"></i>
</a>
</li>
</ul>

View file

@ -75,8 +75,14 @@ export default {
return
}
const differences = Object.fromEntries(
Object.entries(values).filter(
([key, value]) => !_.isEqual(value, this.workflowAction[key])
)
)
// In this case there weren't any actual changes
if (_.isMatch(this.workflowAction, values)) {
if (Object.keys(differences).length === 0) {
return
}
if (values.type) {

View file

@ -26,4 +26,14 @@ export class WorkflowActionType extends Registerable {
getDataSchema(applicationContext, workflowAction) {
throw new Error('Must be set on the type.')
}
/**
* Returns whether the workflow action configuration is valid or not.
* @param {object} workflowAction - The workflow action to validate.
* @param {object} param An object containing application context data.
* @returns true if the workflow action is in error
*/
isInError(workflowAction, { page, element, builder }) {
return false
}
}

View file

@ -17,6 +17,18 @@ describe('elementTypes tests', () => {
const contextBlankParam = { page: { parameters: { id: '' } } }
const misconfiguredOpenPageWorkflowActionPage = {
id: 1,
shared: false,
path_params: [{ name: 'id', type: 'numeric' }],
}
const misconfiguredOpenPageWorkflowAction = {
type: 'open_page',
page_parameters: [],
navigation_type: 'page',
navigate_to_page_id: misconfiguredOpenPageWorkflowActionPage.id,
}
describe('CollectionElementTypeMixin tests', () => {
test('hasAncestorOfType', () => {
const page = { id: 123 }
@ -728,9 +740,9 @@ describe('elementTypes tests', () => {
element.image_file = { url: 'http://localhost' }
expect(elementType.isInError({ element })).toBe(false)
// Image with missing image_url is invalid
element.image_source_type = ''
element.image_url = ''
// Image with image_source_type of 'url' must have an image_url
delete element.image_file
element.image_source_type = IMAGE_SOURCE_TYPES.URL
expect(elementType.isInError({ element })).toBe(true)
// Otherwise it is valid
@ -740,21 +752,50 @@ describe('elementTypes tests', () => {
})
describe('ButtonElementType isInError tests', () => {
test('Returns true if Button Element has no errors, but has misconfigured workflow actions.', () => {
const page = {
id: 2,
shared: false,
name: 'Foo Page',
workflowActions: [
{ ...misconfiguredOpenPageWorkflowAction, element_id: 50 },
],
}
const element = {
id: 50,
value: 'Click me',
page_id: page.id,
}
const builder = {
id: 1,
pages: [page, misconfiguredOpenPageWorkflowActionPage],
}
const elementType = testApp.getRegistry().get('element', 'button')
// Button with value and invalid workflowAction is invalid
expect(elementType.isInError({ page, element, builder })).toBe(true)
})
test('Returns true if Button Element has errors, false otherwise', () => {
const page = { id: 1, name: 'Foo Page', workflowActions: [] }
const page = {
id: 1,
shared: false,
name: 'Foo Page',
workflowActions: [],
}
const builder = { id: 1, pages: [page] }
const element = { id: 50, value: '', page_id: page.id }
const elementType = testApp.getRegistry().get('element', 'button')
// Button with missing value is invalid
expect(elementType.isInError({ page, element })).toBe(true)
expect(elementType.isInError({ page, element, builder })).toBe(true)
// Button with value but missing workflowActions is invalid
element.value = 'click me'
expect(elementType.isInError({ page, element })).toBe(true)
expect(elementType.isInError({ page, element, builder })).toBe(true)
// Button with value and workflowAction is valid
page.workflowActions = [{ element_id: 50 }]
expect(elementType.isInError({ page, element })).toBe(false)
page.workflowActions = [{ element_id: 50, type: 'open_page' }]
expect(elementType.isInError({ page, element, builder })).toBe(false)
})
})
@ -785,8 +826,44 @@ describe('elementTypes tests', () => {
})
describe('FormContainerElementType isInError tests', () => {
test('Returns true if Form Container Element has no errors, but has misconfigured workflow actions.', () => {
const page = {
id: 2,
shared: false,
name: 'Foo Page',
workflowActions: [
{ ...misconfiguredOpenPageWorkflowAction, element_id: 50 },
],
}
const element = {
id: 50,
submit_button_label: 'Submit',
page_id: page.id,
}
const childElement = {
id: 51,
type: 'input_text',
page_id: page.id,
parent_element_id: element.id,
}
page.elementMap = { 50: element, 51: childElement }
page.orderedElements = [element, childElement]
const builder = {
id: 1,
pages: [page, misconfiguredOpenPageWorkflowActionPage],
}
const elementType = testApp.getRegistry().get('element', 'form_container')
// Form container with value and workflowAction is valid
expect(elementType.isInError({ page, element, builder })).toBe(true)
})
test('Returns true if Form Container Element has errors, false otherwise', () => {
const page = { id: 1, name: 'Foo Page', workflowActions: [] }
const page = {
id: 1,
shared: false,
name: 'Foo Page',
workflowActions: [],
}
const element = {
id: 50,
submit_button_label: 'Submit',
@ -794,6 +871,10 @@ describe('elementTypes tests', () => {
}
page.elementMap = { 50: element }
page.orderedElements = [element]
const builder = {
id: 1,
pages: [page, misconfiguredOpenPageWorkflowActionPage],
}
const elementType = testApp.getRegistry().get('element', 'form_container')
@ -801,8 +882,8 @@ describe('elementTypes tests', () => {
expect(elementType.isInError({ page, element })).toBe(true)
// Invalid if we have no children
page.workflowActions = [{ element_id: 50 }]
expect(elementType.isInError({ page, element })).toBe(true)
page.workflowActions = [{ element_id: 50, type: 'open_page' }]
expect(elementType.isInError({ page, element, builder })).toBe(true)
// Valid as we have all required fields
const childElement = {
@ -813,11 +894,11 @@ describe('elementTypes tests', () => {
}
page.elementMap = { 50: element, 51: childElement }
page.orderedElements = [element, childElement]
expect(elementType.isInError({ page, element })).toBe(false)
expect(elementType.isInError({ page, element, builder })).toBe(false)
})
})
describe.only('elementType elementAround tests', () => {
describe('elementType elementAround tests', () => {
let page, sharedPage, builder
beforeEach(async () => {
// Populate a page with a bunch of elements

View file

@ -96,4 +96,23 @@ describe('resolveElementUrl tests', () => {
)
expect(result).toEqual('/builder/123/preview/contact/')
})
test('Should return resolvedContext and ignore element page params when destination page params are removed', () => {
const element = {
navigation_type: 'page',
navigate_to_page_id: 1,
page_parameters: [{ name: 'id', value: '"10"' }],
}
const builder = {
id: 123,
pages: [{ id: 1, path: '/products/', path_params: [] }], // Page parameters have been removed
}
const result = resolveElementUrl(
element,
builder,
builder.pages,
resolveFormula,
'preview'
)
expect(result).toEqual('/builder/123/preview/products/')
})
})