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 language picker #143

Merged
merged 18 commits into from Apr 20, 2021
Merged

Add language picker #143

merged 18 commits into from Apr 20, 2021

Conversation

cblgh
Copy link
Contributor

@cblgh cblgh commented Apr 13, 2021

2021-04-13-163635_1309x1033_scrot
Closes #45

tasks

  • for each translated language, use the language name as defined in its original language
    • hack: add key LanguageName defining the language name of a given translation?
  • get the list of all currently translated languages (for iterating over in template)
  • add a route somewhere for accepting the /change-language POST request
  • do the cookie stuff per details in Make language picking possible #45
  • tweak web/i18n/helper.go to query for any language cookies & use the corresponding language translation if the cookie is set
  • only render language picker when we have >= 2 language translations to pick from
  • add default language setting to admin settings page
  • fix /set-language only working on non-admin pages (bizarre csrf error), fixed in bbd77b4
  • sort language options
  • add bool to settings table; column name use_subdomain_for_aliases with a default of 1
  • test
    • admin: set default language correctly sets language option in database
    • user: posting /set-language sets a language cookie

@staltz
Copy link
Member

staltz commented Apr 13, 2021

@cblgh Can you put this PR in draft mode to indicate its readiness?

@cblgh cblgh marked this pull request as draft April 13, 2021 15:06
@@ -1,5 +1,8 @@
# default localiztion file for english

# the name of this language, in its language. Used for language picking
LanguageName = "English"
Copy link
Contributor Author

@cblgh cblgh Apr 15, 2021

Choose a reason for hiding this comment

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

this new key LanguageName is used for populating the language picker (the list of languages in the page footer). it would be a good idea to document the translation key alongside other instructions for adding translations to the existing room

@cblgh
Copy link
Contributor Author

cblgh commented Apr 16, 2021

this is the test failure i am seeing on my machine:

635.162497ms unit=tunnel called=endpoints
/home/cblgh/code/go/src/go-ssb-room/muxrpc/test/nodejs/testscripts/modern_aliases.js:12
      sbot.roomClient.registerAlias(rpc.id, "alice", (err, ret) => {
                      ^

TypeError: Cannot read property 'registerAlias' of undefined
    at Timeout._onTimeout (/home/cblgh/code/go/src/go-ssb-room/muxrpc/test/nodejs/testscripts/modern_aliases.js:12:23)
    at listOnTimeout (internal/timers.js:554:17)
    at processTimers (internal/timers.js:497:7)
level=debug t=5.541728553s event="network listen loop exited"
level=debug t=5.542631243s event="tunserv closing" msg="connections closed"
level=info t=5.542693387s event="tunserv closing" msg="closers closed"
--- FAIL: TestGoServerJSClientAliases (5.54s)
    setup_test.go:131: go server: @7j2jowwuHPQd5YRes3smYRKNrd3+uKteXdILPzjBfYQ=.ed25519
    setup_test.go:161: starting client alice
    setup_test.go:217: JS alice:1 @yVFwS8u/+Uiu6I1eJbfBaQtx70EVmrwnhSCmYO6Iy3k=.ed25519
    setup_test.go:199: node client alice: exited with exit status 1 (after 1.592460342s)
    setup_test.go:280: 
        	Error Trace:	setup_test.go:280
        	            				aliases_test.go:45
        	Error:      	Received unexpected error:
        	            	node client alice exited with exit status 1
        	Test:       	TestGoServerJSClientAliases
    aliases_test.go:47: 
        	Error Trace:	aliases_test.go:47
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestGoServerJSClientAliases
        	Messages:   	register call count
    setup_test.go:133: bot close: <nil>

@cblgh
Copy link
Contributor Author

cblgh commented Apr 16, 2021

2021-04-16-090351_1419x1059_scrot

working in parallel on the default language admin settings, it's uncovered some additions i need to make to the template function list_languages :)

@cryptix
Copy link
Member

cryptix commented Apr 16, 2021

re the test failure, i just checked out your branch and couldn't reproduce. I suspect the npm deps changed.

Try running cd muxrpc/test/nodejs && npm ci && go test -run TestGoServerJSClientAliases

@staltz
Copy link
Member

staltz commented Apr 16, 2021

The test failure means that ssb-room-client wasn't installed properly, so just double check that it's in node_modules, the version is the latest, and in secret-stack we're doing the .use(require('ssb-room-client'))

@cblgh
Copy link
Contributor Author

cblgh commented Apr 16, 2021

ship this german translation PRONTO
2021-04-16-155020_1256x639_scrot

@cblgh
Copy link
Contributor Author

cblgh commented Apr 16, 2021

ok, all that's remaining then is

  • fix /set-language only working on non-admin pages (bizarre csrf error)

@cryptix
Copy link
Member

cryptix commented Apr 19, 2021

Totally orthogonal but before we need yet another migration, can you add another bool to the settings table for #160 ?

column name use_subdomain_for_aliases and default it to 1.

Update models please and I will add the functions on the roomdb interfaces myself later. Just don't want to touch that migration file again.

@cblgh
Copy link
Contributor Author

cblgh commented Apr 19, 2021

@cryptix gotcha! will do this after the current batch of work i'm doing :)

@cblgh
Copy link
Contributor Author

cblgh commented Apr 19, 2021

5a15cab look alright @cryptix?

@cblgh cblgh marked this pull request as ready for review April 19, 2021 19:13
i18n.ListLanguages() returns a map, mapping language tags ('en', 'sv')
to the names of their corresponding languages (as translated by the
language itself).

This functionality will be used in the language picker, to present a
nice list of the translated languages.

I renamed testing.go to conform to go's testing conventions, and
added a test for i18n.ListLanguages().
while working on the /set-language route, i noticed that i was getting a
csrf error for all /admin views when setting the language, while it
worked well on non-admin routes.

the issue, it turned out, was that we needed to configure gorilla's csrf
feature to set all cookies on the same route. when unconfigured, the
set cookies will only be set for the path they are being set at.

see more in the gorilla.csrf documentation (in particular the csrf.Path
option): https://pkg.go.dev/github.com/gorilla/csrf?utm_source=godoc#Path
to make sure the list of languages is sorted, we now use a slice of
TagTranslation{Tag: string, Translation: string} structs, sorted
by `TagTranslation.Tag`.
Copy link
Member

@cryptix cryptix left a comment

Choose a reason for hiding this comment

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

Some nagging but really like the substance! Good for merge after these changes.

roomdb/sqlite/roomconfig.go Outdated Show resolved Hide resolved
web/handlers/admin/set_language_test.go Outdated Show resolved Hide resolved
web/handlers/admin/settings.go Outdated Show resolved Hide resolved
web/handlers/admin/settings.go Outdated Show resolved Hide resolved
web/handlers/http.go Outdated Show resolved Hide resolved
web/handlers/http.go Outdated Show resolved Hide resolved
web/handlers/http.go Outdated Show resolved Hide resolved
web/handlers/invites_test.go Outdated Show resolved Hide resolved
web/handlers/notices_test.go Outdated Show resolved Hide resolved
web/handlers/set_language_test.go Outdated Show resolved Hide resolved
@cblgh
Copy link
Contributor Author

cblgh commented Apr 20, 2021

Thanks for the code review @cryptix! I'll get on implementing the remaining changes & fixing the tests :)

Co-authored-by: Henry <111202+cryptix@users.noreply.github.com>

use eh.Handle
@cblgh cblgh force-pushed the language-picking branch 4 times, most recently from 42f6cce to a159108 Compare April 20, 2021 09:45
Copy link
Member

@cryptix cryptix left a comment

Choose a reason for hiding this comment

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

Very nice now! LGTM

@cblgh cblgh merged commit 2007c73 into master Apr 20, 2021
@cblgh cblgh deleted the language-picking branch April 20, 2021 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make language picking possible
3 participants