search

ether / ep_comments_page

Comments in Etherpad - No Longer Highly Experimental, now highly awesome!
Apache License 2.0
45 stars 48 forks source link

add export setting #178

Closed ilmartyrk closed 3 years ago

ilmartyrk commented 3 years ago

As this HTML export seems broken and we at out project don't want to export HTML anyway I added a setting option to skip export. Default behaviour is exporting but if settings.ep_comments_page.exportHTML = false; export is skipped

JohnMcLear commented 3 years ago

Which issue has broken export???

JohnMcLear commented 3 years ago

Also won't this fail if no settings for this plugin are set??

ilmartyrk commented 3 years ago

@JohnMcLear I don't see extra content getting exported on our app, but I haven't dug too deep. I only see the asterisks as link to comment, but don't see the comment paragraph, also tests didn't pass. It might be only my implementation fault and I might have some settings or smth. missing. Anyway I would still like to have option to skip out on export :)

JohnMcLear commented 3 years ago

Totally fine to have option but plz create issue with bug so we are aware about that too :)

ilmartyrk commented 3 years ago

@JohnMcLear I added the check if (settings.ep_comments_page && settings.ep_comments_page.exportHtml === false) return; so if no settings are set nothing breaks.

JohnMcLear commented 3 years ago

Please as last step add backend test coverage.

One last thing to manually check is that if you have settings block but don't define this Boolean does it still export comments in html?

ilmartyrk commented 3 years ago

@JohnMcLear I tried to add test, but don't know how to add Settings into test. Tested manually and it works.

ilmartyrk commented 3 years ago

Should I make a test dependent on settings.json change so that it will by default skip, but will succeed if exportHTML setting is false and by doing this others will fail?

JohnMcLear commented 3 years ago

You can make settings be modified in real time as you run the test, for example: https://github.com/ether/etherpad-lite/blob/develop/tests/backend/specs/webaccess.js#L22

Test once with the setting set to true, make sure content is as you expect, and once with setting set to false.

JohnMcLear commented 3 years ago

Awesome thanks :)

One absolutely final thing, sorry! Please include docs in README.md :)

ilmartyrk commented 3 years ago

@JohnMcLear updated README.md also fixed the export html tests

JohnMcLear commented 3 years ago

<3 :)

JohnMcLear commented 3 years ago

Awesome work thanks dude!