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

add form title to archived notifications #1065

Closed
wants to merge 1 commit into from

Conversation

lemondevxyz
Copy link
Contributor

@lemondevxyz lemondevxyz commented Jun 25, 2023

Fixes #1060

Notifications in CryptPad have two versions AFAIK, one that sends the notification to the active WebSocket and one that is stored in ./datastore/{HASH}/{HASH}.ndjson

Theoretically, the notifications sent by ./www/form/inner.js are privitized by the server when archived. I haven't confirmed this but it seems that the server, when sending the websocket notification, either fetches the form's data or removes it when storing the notification in data store. (your guess is as good as mine)

Anyhoo, this fix is a bit hacky since it just adds a title field to the notification:

// ./www/form/inner.js:3597
// previous
msgData: { channel: priv.channel }
// current
msgData: { channel: priv.channel, title: framework._.title.title || framework._.title.defaultTitle }

Then, it assigns the title in the "right" place because a title in the data field of a notification will not be used.

// ./www/common/sframe-common/mailbox.js:270
// previous
messages.push({
	// ... other fields
	content: {msg: data.message, time: data.time, hash: data.hash},
	// ... other fields
})
// current
messages.push({
	// ... other fields
	content: {
		// ... other fields
		msg: Object.assign(data.message, {
			content: Object.assign(data.message.content, {
				title: data.message.content.data.title,
			})
		},
		// ... other fields
	}
	// ... other fields
})

(above code examples are not 100% accurate and are used for presentation purposes...)

…/inner.js

Notifications in CryptPad have two versions AFAIK, one that sends the
notification to the active WebSocket and one that is stored in
./datastore/{HASH}/{HASH}.ndjson

Theoretically, the notifications sent by ./www/form/inner.js are
privitized by the server when archived. I haven't confirmed this but
it seems that the server, when sending the websocket notification,
either fetches the form's data or removes it when storing the
notification in data store. (your guess is as good as mine)

Anyhoo, this fix is a bit hacky since it just adds a title field to
the notification:
```js
// ./www/form/inner.js:3597
// previous
msgData: { channel: priv.channel }
// current
msgData: { channel: priv.channel, title: framework._.title.title || framework._.title.defaultTitle }
```

Then, it assigns the title in the "right" place because a title in the
data field of a notification will not be used.
```js
// ./www/common/sframe-common/mailbox.js:270
// previous
messages.push({
	// ... other fields
	content: {msg: data.message, time: data.time, hash: data.hash},
	// ... other fields
})
// current
messages.push({
	// ... other fields
	content: {
		// ... other fields
		msg: Object.assign(data.message, {
			content: Object.assign(data.message.content, {
				title: data.message.content.data.title,
			})
		},
		// ... other fields
	}
	// ... other fields
})
```

(above code examples are not 100% accurate and are used for
presentation purposes...)
@davidbenque davidbenque changed the base branch from main to staging June 26, 2023 12:47
@davidbenque davidbenque requested a review from yflory June 26, 2023 12:48
@davidbenque davidbenque added this to the 5.4.0 milestone Jun 29, 2023
@ghost ghost added Form Related to the Form app UI/UX Interfaces & user experience labels Jul 5, 2023
Copy link
Contributor

@wginolas wginolas left a comment

Choose a reason for hiding this comment

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

I tested this and it looks good. @yflory, can you have a look at the code?

@yflory
Copy link
Contributor

yflory commented Jul 11, 2023

Hi,
While reviewing your pull request, I found an underlying issue with all notifications from the history which explains the difference between "live notifications" and history. The part of the client that was supposed to add the most recent "title" by searching through the user's drive was not called! I'm currently testing a fix.

Notifications in CryptPad have two versions AFAIK, one that sends the notification to the active WebSocket and one that is stored in ./datastore/{HASH}/{HASH}.ndjson

Theoretically, the notifications sent by ./www/form/inner.js are privitized by the server when archived. I haven't confirmed this but it seems that the server, when sending the websocket notification, either fetches the form's data or removes it when storing the notification in data store. (your guess is as good as mine)

FYI, the server has nothing to do with this, it only sees encrypted data. Everything is done in the client!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Form Related to the Form app UI/UX Interfaces & user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Form title is missing on notifications page
4 participants