Have client read out shared settings from server #311

pellenilsson · 2017-03-19T17:24:56Z

Some settings (reply-to-self and require-email) always need to be set to the same value on server and client side for correct operation. This change removes the need for such redundant information by having the client read out those settings from the server.

According to the docs this should be true for require-author as well, but that settings doesn't seem to be implemented.

brad · 2018-12-25T16:05:12Z

This is brilliant! require-author is working here, maybe that should be part of the PR as well. I think also documentation changes should be made before this can be merged

jelmer · 2018-12-27T01:14:24Z

Any chance you could resolve the conflicts?

pellenilsson · 2018-12-27T23:05:41Z

In my fork, I took this a step further and centralized all settings to the server side (see ed56921).

The main content of my fork is authentication with Facebook, Google and OpenID as described in this blog post which also has a live demo.

I was earlier under the impression that this line of work was not desired in mainline Isso, but since this may have changed we could have a discussion about what functionality to include before looking at the conflicts.

jelmer · 2018-12-27T23:27:33Z

When you say "this line of work", do you mean all of your fork or specifically the exposing of server settings to the client? I'm not aware of a reason why the latter would be controversial.

pellenilsson · 2018-12-27T23:34:33Z

I meant the entire fork. I just want to avoid multiple merges by deciding what to include.

jelmer · 2018-12-27T23:46:57Z

I think either way we'd want to merge those changes one by one rather than all at once.

pellenilsson · 2018-12-28T12:44:49Z

Yes, moving settings is a quite separate change of course. But i thought we could agree on moving all settings or just the "redundant" settings that now have to be specified on both server and client side.

On the other hand, moving only the "redundant" settings is easier because there are no backward compatibility issues. Moving all settings would either require supporting the old way as well, or forcing users to move their settings.

blatinier · 2018-12-28T12:52:09Z

Bumping isso version to 2.x.x might be enough to indicate breaking change don't you think?
The centralization of config is really neat and would be very much appreciated since it would simplify the process of installing/setting up isso.

blatinier · 2018-12-28T12:56:19Z

I fixed conflicts, and it should be ok. Needs a bit of testing though.

pellenilsson · 2018-12-28T13:23:23Z

Great! So I might create a new pull request for centralizing all config when I find the time.

TheKerbycz · 2019-01-02T18:08:32Z

Hi, I was going to use the pellenilsson's fork with social auth, but after reading this conversation I'm a not really sure what version (and branch) should I use. Should I use branch 'feature/get-server-config' or 'master'? Am I right, that the social login feature is only in this fork? If so, are there any plans to include it also in the posativ's repository? Thanks

pellenilsson · 2019-01-05T00:02:16Z

Hi @TheKerbycz! The social auth stuff is only available in the master branch of my forked repo https://github.com/pellenilsson/isso. Since it was forked some time ago it misses some of the newer features. Whether it will be merged to posativ's repo is an open question.

jelmer · 2021-12-22T19:27:24Z

This seems reasonable to me; As it has no unit tests, has anybody else managed to successfully test it?

ix5 · 2022-01-01T16:45:56Z

@pellenilsson would you mind rebasing this on top of current master?

While you're at it, there are a few more settings that are shared between client and server and need to be kept in sync. I've quickly glanced over the docs and found these:

data-isso-require-author with [guard] require-author
data-isso-require-email with [guard] require-email
data-isso-reply-notifications with [general] reply-notifications
data-isso-gravatar with [general] gravatar

Also, it'd be nice for the "gravatar" setting to trump the regular avatars if set to true instead of having both generated.

@pellenilsson if you're done with the shared settings, we'd need to document this new behaviour as well, first in CHANGES and then also in the docs. I'd be happy to help you there.

Just additionally, this is wildly out of scope, but we might want to display the available HTML tags and markdown options to clients in some kind of way. So exposing those would fall under the same mechanism.

pellenilsson · 2022-01-04T08:28:43Z

@ix5 I merged in latest master and added the additional settings that you mentioned, and tried to handle the gravatar scenario. I also updated the docs.

But I no longer have a setup for testing this out since I haven't worked on Isso for several years, so it would be nice if someone else could do some testing.

ix5

Left a few comments. Thanks for rebasing and addressing those issues. I'll try to test thoroughly once I find the time.

isso/js/embed.js

docs/docs/configuration/client.rst

ix5 · 2022-01-04T18:02:55Z

Pinging @stefangehn @Guts @zackw @vsajip @Konzertheld for testing/review. Would be nice of one of you to take a look at this!

The following settings are sent as a config object from the server to the client: - `reply-to-self` - `require-author` - `require-email` - `reply-notifications` - `gravatar` This means that the following settings will be ignored when set on the client side: - `data-isso-reply-to-self` - `data-isso-require-author` - `data-isso-require-email` - `data-isso-reply-notifications` - `data-isso-gravatar` Isso will notify the user on the browser console if a client setting has been overwritten by a config from the server. Gravatar setting trumps regular avatars: In addition, `data-isso-avatar` will be set to `false` when gravatars are enabled to prevent both regular avatars and gravatars appearing side-by-side. Documentation updates: Update docs for shared server-client settings and clarify documentation for gravatar/avatar settings

ix5 · 2022-01-08T11:16:51Z

@pellenilsson currently, we're in a bit of a catch-22: fetchComments() checks if #isso-root is present, but fails since the call to create a new h4 header, a Postbox and the div#isso-root element is part of fetchComments() itself.

The fix for that is to add the header and root element early, but only later insert the postbox. I'm not aware of any issues concerning the postbox being inserted after the root element.

I've pushed to your branch with a few suggestions and improvements. If you like, you can reset your HEAD to https://github.com/ix5/isso/tree/pull-311-rebased , I've squashed the commits for you already there and written a succinct commit message.

pellenilsson · 2022-01-10T11:55:22Z

I've pushed to your branch with a few suggestions and improvements. If you like, you can reset your HEAD to https://github.com/ix5/isso/tree/pull-311-rebased , I've squashed the commits for you already there and written a succinct commit message.

Thanks! I force-pushed your squashed branch now.

ix5 · 2022-01-24T11:26:22Z

@pellenilsson I haven't forgotten your PR, I am just trying to think of ways to verify this does not break anything.
Will try to get to it during the remainder of this week.

ix5 · 2022-02-06T21:04:23Z

Tested various configuration disparities between client and server and the server always takes precedence.
I couldn't spot any obvious bugs or oversights.

The warning logic is off for when default client settings (from config.js) are overridden by server settings/defaults. But that's for another day to fix, this PR has languished in limbo way too long.

Thank you so much @pellenilsson for this great improvement!

The following settings are sent as a config object from the server to the client: - `reply-to-self` - `require-author` - `require-email` - `reply-notifications` - `gravatar` This means that the following settings will be ignored when set on the client side: - `data-isso-reply-to-self` - `data-isso-require-author` - `data-isso-require-email` - `data-isso-reply-notifications` - `data-isso-gravatar` Isso will notify the user on the browser console if a client setting has been overwritten by a config from the server. Gravatar setting trumps regular avatars: In addition, `data-isso-avatar` will be set to `false` when gravatars are enabled to prevent both regular avatars and gravatars appearing side-by-side. Documentation updates: Update docs for shared server-client settings and clarify documentation for gravatar/avatar settings

ix5 · 2022-02-06T21:06:57Z

@pellenilsson in case you still feel like contributing after such a long wait, have a look at https://github.com/ix5/isso/commits/pull-311-preserve-defaultconf for slightly correcting the warnings in the console.

Also, as a note to myself: Sending the config object with every request is probably not too expensive, but we should still only transfer it once.

This restores the behavior before isso-comments#311 Closes isso-comments#815 Note: This is only a hotfix!

This is a backport of #820 to the 0.12.6 release branch. This restores the behavior before #311 Closes #815 Note: This is only a hotfix!

This is an improvement on top of: 02f3ea0 "Have client read out shared settings from server (isso-comments#311)" So far, the `config` dict was being sent with the server response when fetching comments from the `fetch()` `/` (GET) endpoint. Create new endpoint at `/config`, which will serve a JSON representation of the server's `public_conf`. Also remove the `config` object from `fetch()` responses. Extend the JS client api file to fetch from the `/config` endpoint, which will only happen once via the `fetchComments` hook on page load.

This restores the behavior before #311 Closes #815 Note: This is only a hotfix!

This is an improvement on top of: 02f3ea0 "Have client read out shared settings from server (isso-comments#311)" So far, the `config` dict was being sent with the server response when fetching comments from the `fetch()` `/` (GET) endpoint. Create new endpoint at `/config`, which will serve a JSON representation of the server's `public_conf`. Also remove the `config` object from `fetch()` responses. Extend the JS client api file to fetch from the `/config` endpoint, which will only happen once via the `fetchComments` hook on page load.

This is an improvement on top of: 02f3ea0 "Have client read out shared settings from server (isso-comments#311)" So far, the `config` dict was being sent with the server response when fetching comments from the `fetch()` `/` (GET) endpoint. Remove the `config` object from `fetch()` responses and instead let client user the newly created `/config` endpoint. Extend the JS client api file to fetch from the `/config` endpoint, which will only happen once via the `fetchComments` hook on page load.

This is a backport of isso-comments#820 to the 0.12.6 release branch. This restores the behavior before isso-comments#311 Closes isso-comments#815 Note: This is only a hotfix!

pellenilsson mentioned this pull request Mar 3, 2019

Authentication using social networks #240

Open

ix5 mentioned this pull request Dec 22, 2021

Please add more maintainers #752

Closed

ix5 added client (Javascript) client code and CSS feature server (Python) server code labels Jan 1, 2022

ix5 self-assigned this Jan 4, 2022

ix5 reviewed Jan 4, 2022

View reviewed changes

isso/js/embed.js Outdated Show resolved Hide resolved

docs/docs/configuration/client.rst Show resolved Hide resolved

pellenilsson force-pushed the feature/get-server-config branch from 4519dbb to 6bf98b0 Compare January 10, 2022 11:53

ix5 merged commit 02f3ea0 into isso-comments:master Feb 6, 2022

ix5 added a commit to ix5/isso that referenced this pull request Mar 20, 2022

isso: js: embed: Insert Postbox before comments

5434ff0

This restores the behavior before isso-comments#311 Closes isso-comments#815 Note: This is only a hotfix!

ix5 mentioned this pull request Mar 20, 2022

isso: js: embed: Insert Postbox before comments #820

Merged

ix5 added a commit that referenced this pull request Mar 20, 2022

isso: js: embed: Insert Postbox before comments

cd75cc4

This is a backport of #820 to the 0.12.6 release branch. This restores the behavior before #311 Closes #815 Note: This is only a hotfix!

ix5 mentioned this pull request Mar 20, 2022

[client] js: Fetch public conf once from /config; embed.js: Preserve default config values #821

Merged

ix5 added a commit that referenced this pull request Mar 21, 2022

isso: js: embed: Insert Postbox before comments (#820)

861f64f

This restores the behavior before #311 Closes #815 Note: This is only a hotfix!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have client read out shared settings from server #311

Have client read out shared settings from server #311

pellenilsson commented Mar 19, 2017

brad commented Dec 25, 2018

jelmer commented Dec 27, 2018

pellenilsson commented Dec 27, 2018

jelmer commented Dec 27, 2018

pellenilsson commented Dec 27, 2018

jelmer commented Dec 27, 2018

pellenilsson commented Dec 28, 2018

blatinier commented Dec 28, 2018

blatinier commented Dec 28, 2018

pellenilsson commented Dec 28, 2018

TheKerbycz commented Jan 2, 2019

pellenilsson commented Jan 5, 2019

jelmer commented Dec 22, 2021

ix5 commented Jan 1, 2022

pellenilsson commented Jan 4, 2022

ix5 left a comment

ix5 commented Jan 4, 2022

ix5 commented Jan 8, 2022

pellenilsson commented Jan 10, 2022

ix5 commented Jan 24, 2022

ix5 commented Feb 6, 2022

ix5 commented Feb 6, 2022 •

edited

Have client read out shared settings from server #311

Have client read out shared settings from server #311

Conversation

pellenilsson commented Mar 19, 2017

brad commented Dec 25, 2018

jelmer commented Dec 27, 2018

pellenilsson commented Dec 27, 2018

jelmer commented Dec 27, 2018

pellenilsson commented Dec 27, 2018

jelmer commented Dec 27, 2018

pellenilsson commented Dec 28, 2018

blatinier commented Dec 28, 2018

blatinier commented Dec 28, 2018

pellenilsson commented Dec 28, 2018

TheKerbycz commented Jan 2, 2019

pellenilsson commented Jan 5, 2019

jelmer commented Dec 22, 2021

ix5 commented Jan 1, 2022

pellenilsson commented Jan 4, 2022

ix5 left a comment

Choose a reason for hiding this comment

ix5 commented Jan 4, 2022

ix5 commented Jan 8, 2022

pellenilsson commented Jan 10, 2022

ix5 commented Jan 24, 2022

ix5 commented Feb 6, 2022

ix5 commented Feb 6, 2022 • edited

ix5 commented Feb 6, 2022 •

edited