Account created on 26 September 2008, over 16 years ago
  • Senior Developer / CTO at werk21 
#

Merge Requests

More

Recent comments

🇩🇪Germany jan kellermann

It is not possible to give a order of purposes for Klaro! JS. The order cannot be guaranteed by a different sequence of services, as a service can have several purposes.

We have to add the order of purposes to DrupalSettings and order the purposes by javascript.

🇩🇪Germany jan kellermann

Bump. Updated fork, ready to review. Would be great to have this issue reviewed and merged for next release.

🇩🇪Germany jan kellermann

Bump. Updated fork, ready to review. Would be great to have this issue reviewed and merged for next release.

🇩🇪Germany jan kellermann

Merged. Thank you for testing and feedback.

🇩🇪Germany jan kellermann

Thank you for feedback and testing.

I have added the information to the readme file.

I don't expect any security implications if we don't check whether the module is installed. If someone has access to the code and can manipulate field values, they can also inject code elsewhere. We just do the rendering and let the Klaro helper check the result.

So I set the state to RTBC.

🇩🇪Germany jan kellermann

Thank you for testing and feedback!
I merged the MR and will release soon a new version.

🇩🇪Germany jan kellermann

I added applyConsents() while initialization.

Please review and feedback.

🇩🇪Germany jan kellermann

I added a link to the consent manager in contextual dialog.

Please review and feedback.

🇩🇪Germany jan kellermann

Thank you for your feedback.

We were not aware of this error message.

I have created a merge request that always displays the title, but hides it visually by default (it can be displayed optionally). So it should work as a label for the dialog.

I changed category and priority.

Please test and provide feedback. Thank you very much!

🇩🇪Germany jan kellermann

The same topic is discussed in #2925718. But it seems, that D11 has solved the problem if session_write_interval is set correctly.

🇩🇪Germany jan kellermann

Thank you very much for this issue and the MR.

The parameter data-tcf-enabled should not be hard-coded, there is a setting fot this paramater and will be added optionally in line 106ff.

In most cases the consent dialog should be in browser's language, so this should be implemented as an adjustable option.

🇩🇪Germany jan kellermann

Added prefix (and trait) to queues. Tested:

redis-cli KEYS *update_fetch_tasks:counter
1) "drupal:queue:update_fetch_tasks:counter"

redis-cli FLUSHALL

redis-cli KEYS *update_fetch_tasks:counter
1) "iFq2Dg:drupal:queue:update_fetch_tasks:counter"

Please review and feedback.

🇩🇪Germany jan kellermann

jan kellermann changed the visibility of the branch 8.x-1.x to hidden.

🇩🇪Germany jan kellermann

jan kellermann made their first commit to this issue’s fork.

🇩🇪Germany jan kellermann

FYI: I am not currently depending on the patch, because getProvidersForOperationType() does not test whether the OperationType has been registered before.

I can simply add any identifiers in my provider's getSupportedOperationTypes().

See AiProviderPluginManager::getProvidersForOperationType().

🇩🇪Germany jan kellermann

Thanks for the feedback (I didn't expect a deliberate removal either :) ).
I have hidden the MR!414 and set the ticket to RTBC.

🇩🇪Germany jan kellermann

jan kellermann changed the visibility of the branch 3499361-ckeditor-does-not to hidden.

🇩🇪Germany jan kellermann

Behat/Mink is predefined in Drupals PHPunit-Config. For local testing you need a chromium. We are running this via docker, see here .
See also this pipeline (line 68ff)
See also this commit.

🇩🇪Germany jan kellermann

Therefore, I created a new fork and MR!417 that only fixes the bug and does not make any other changes in code or function.

Feel free to test and give feedback. Thanks a lot.

🇩🇪Germany jan kellermann

MR solves the problem for me. Unfortunately, MR still makes changes to the code that are not directly related, for example removing the 3rd parameter when calling the chat() function. Therefore I cannot give an RTBC.

🇩🇪Germany jan kellermann

@marcus_johansson I got stuck in communication here, sorry.

Yes, AnythingLLM is a RAG out of the box and so no actions are needed in the module. So it would be good if the array was queried beforehand.

I added this commit ("You may need..." instead of "You will need...").

I have updated the fork.

For me the patch works. Because I am not working with actions I did not set RTBC.

🇩🇪Germany jan kellermann

> Could you provide any insights into why you wanted a new operation type?

AnythingLLM provides ONE endpoint for embedding AND storing embedded data in vector database. AnythingLLM is an abstraction layer also (you can choose which LLM for embedding and which VDB for storing the data). This means we cannot separate embedding and saving in the vector database.

🇩🇪Germany jan kellermann

@akulsaxena: Thank you for feedback. I added operationtype to cspell because the hook name "operation_type" would be misleading.

@marcus_johansson: Thank you again for feedback and the strategic thinking. We need this for search-api because AnythingLLM directly provides an endpoint for the indexing (embedding incl. vdb).

🇩🇪Germany jan kellermann

I added the invoke call.

Please review and feedback.

Thank you!

🇩🇪Germany jan kellermann

For testing you can use the /user/password page without further config (only enable antibot) and I uploaded webform.webform.test_3406484.yml which you may import and use for multi-page webform tests.

🇩🇪Germany jan kellermann

In general the scope of this issue moved. The webform-bug seems to be fixed without this issue.

Interesting is comment #5 🐛 Antibot blocks multistep webform submission if user fails to move mouse pointer Active where several new a11y problems are mentioned that call into question the functioning of the module in general for accessibility reasons.

The MR!23 tries to solve this problems and attempts to balance accessibility and spam protection.

🇩🇪Germany jan kellermann

@danrod: Thank you for testing. The patch antibot-issue-3406484-2.patch you used is almot 13 months old. Please test with current issue fork.

I changed the test to fill in the form data via JS instead of using Mink because Mink simulates the keystroke events.

🇩🇪Germany jan kellermann

Test added.

Default tests ran successfully.

Default Test-only failed as expected:

There was 1 failure:
1) Drupal\Tests\user\FunctionalJavascript\UserRegisterFormTest::testRegistrationFormStorage
Written not strictly necessary Drupal.visitor.name to localStorage without consent.
Failed asserting that false is true.

Please review.

🇩🇪Germany jan kellermann

There are other behaviors in the form.js file besides fillUserInfoFromBrowser.

I have not checked if the forms use other behaviors (e.g. formSingleSubmit), but would recommend to deprecate the attachment later, as contrib modules or custom code could use behaviors from this file.

🇩🇪Germany jan kellermann

Die stabile Version 3.0 ist veröffentlicht! Ich habe eben die letzten Terms übersetzt.

Danke für Eure Unterstützung und Euren Zuspruch auf dem Weg zu dieser Version.

🇩🇪Germany jan kellermann

We just released Klaro! 3.0.0 stable release .

Thank you all for this journey the last weeks - it was awesome!

🇩🇪Germany jan kellermann

I created the new child issue #3498834 🐛 Dont use core's prepopulate function for core forms (Privacy) Active with issue description according to template.

I also created a new change record. The old change record from this issue can be deleted.

I hope that this will speed up the process.

I set priority to normal because depecration is not as important as getting the core compliant with data protection laws.

🇩🇪Germany jan kellermann

jan kellermann changed the visibility of the branch 3498834-dont-use-cores to hidden.

🇩🇪Germany jan kellermann

Thank you for MR! We remove the whole library because it is not used.

🇩🇪Germany jan kellermann

The bug occurs in the context module due to the missing assignment of IDs. This is solved in issue #3277701.

Therefore, I think this issue can be closed as a wontfix.

🇩🇪Germany jan kellermann

Issue fork Updated to 5.x - maintainers should change merge target to 5.x

Tested and RTBC.

Thank you for the patch.

🇩🇪Germany jan kellermann

jan kellermann made their first commit to this issue’s fork.

🇩🇪Germany jan kellermann

I was also not able to reproduce this issue on the 5.x branch when using field_group 3.6 and Drupal 10.3.

Please give more information to reproduce.

🇩🇪Germany jan kellermann

D7 is EOL. I tested on D11 with current 2.x branch and could not reproduce on Safari on iOS18.

If this problem occurs further, we should create a patch for 2.x.

🇩🇪Germany jan kellermann

Antibot is more effective after this change than before, but can still be bypassed. If I have understood correctly, Antibot is bypassed by saving and transferring the key. This means that we need more variance in the key.

Suggestion:

Generate the antibot key using the form_token (does webform actually use form_token? I couldn't find one, in this case use the form_build_id). However, this would mean that the antibot key is generated and transmitted per form and no longer per page (but IMHO this does not require any major adjustment, there should also be no differences in caching, because the cache duration is analogous to form_token or form_build_id). Was there a reason why a separate key was generated back then?

Remark:

In issue #3406484, a condition was added as to whether an event isTrusted, which may also improve the quality.

🇩🇪Germany jan kellermann

The solution of allowing clicks and querying the isTrusted attribute can significantly reduce the effectiveness of the module. We have therefore created a new merge request that adds an adjustable time component (default 5 seconds) to the click event.

Please review / feedback.

🇩🇪Germany jan kellermann

Set to RTBC after final discussing on slack.
Just resolving last stylelint issues and then merge.

Thank you all for feedback and testing. We couldn't have done it without your support!

🇩🇪Germany jan kellermann

How do you think OSM is loaded? The Leaflet module is already supported. We have to interrupt the integration, so we need information about the integration

And which URLs should we integrate? Many countries have their own URLs that can be integrated via iframe, for example. See also this list: https://wiki.openstreetmap.org/wiki/Using_OpenStreetMap#Maps

🇩🇪Germany jan kellermann

Danke nochmal! Die v1 kommt hoffentlich in den nächsten 7 Tagen, einige neue Translations kommen dann noch, aber ich mache Euch Vorschläge :)

🇩🇪Germany jan kellermann

Many thanks for the comprehensive and accurate feedback again. It is a great pleasure to work with you!

title and description

Changed in code and README now. ("also" is correct because there is a standard-style with drupal colors and additional three styles for the known themes).

Olivero

notice: (windows high contrast)

> the notice dialog has no focus outline

Fixed.

consent (windows high contrast mode)

> the toggles dont have a focus outline.

On windows they have a 2px dotted white outline (I removed now the transition of 0,4 seconds).

Claro:

consent (windows high contrast mode)

> the close button is not autofocused like without the WHCM

On windows the outline is visible.

Gin:

windows high contrast mode

I force now the outline for links.

Also done:

  • Removed transition from slider buttons.
  • Added reverse icon for high contrast mode.

Thanks for the suggestion to chat directly and clarify what needs to be done before the merge.

🇩🇪Germany jan kellermann

Ihen I'll postpone this issue and we'll wait as suggested for specific descriptions of the requirements from @schwankde.

As there has been no feedback for 2 months, I set the priority to normal.

🇩🇪Germany jan kellermann

Merged. Thank all for feedback and testing. Close this issue.

🇩🇪Germany jan kellermann

Could not reproduce.

  • I installed a new D10.4 and required drupal/geolocation:^3.14.
  • I enabled geolocation_leaflet (drush automatically installed all further modules.)
  • I added a new geo field to content type basic page with display Geolocation Formatter - Map without any further options.
  • I created a new node with a geolocation.
  • I viewed this node as anonymous user with and without JS aggregation and also with multiple drush cr.

Can you provide more information? Maybe a screenshot of network tab in your browsers DevTools?

🇩🇪Germany jan kellermann

I also adjusted the Drupal versions to ^10.2 || ^11.

🇩🇪Germany jan kellermann

Worked on #3496782 and did not see that you resolved this issue before in patch #6.

Because for #3496782 the MR!11 is already created and most other changes already part of 3.x-dev branch, I only tested the last open topic: Fixing the paragraph edit form (disabling the checkbox was not possible).

Your patch resolved the problem so I opened MR!12. With these two MRs a D11 release should be possible.

Thank you for all your work on this great module.

🇩🇪Germany jan kellermann

Could reproduce WSOD and the patch fixed the problem.

I created a MR (with a shortened category description).

🇩🇪Germany jan kellermann

jan kellermann made their first commit to this issue’s fork.

🇩🇪Germany jan kellermann

I am working to implement as you suggested.

What is done:

- shorten the expiration of privateStorage to 24h.
- adding ProviderProxy to AiAssistantApiRunner because it is used multiple times.
- initializing provider ad threadID in AiAssistantApiRunner::setAssistant().
- removed separate logic for custom threadID-naming and garbage collector because of shorter expiration time.
- AiAssistantApiRunner::generateUniqueHash() gets sessionID from provider.
- AiAssistantApiRunner::setThreadsKey() sets the sessionID in provider also.
- AiAssistantApiRunner::unsetThreadsKey() resets the sessionID in provider also.
- AiAssistantApiRunner::getTempStore() now uses the threadID as prefix and returns directly data array (adjusted all calls to getTempStore()).
- Added AiAssistantApiRunner::setTempStore() and AiAssistantApiRunner::deleteTempStore().

But I struggle with the mixup of threadID/sessionID-setting and storage because in my opionion they should be independent from each other. I suggest to split this option up to in AssistantAPI settings form:

SessionID:
- use always the same sessionID
- use for every chat a new sessionID

History:
- no history
- store in session
- store in database

And for deepchat I suggest to use the deepchat history and make this configurable in block settings. You can make the number of message configurable:

    $deepchat['requestBodyLimits'] = [
      'maxMessages' => 10,
    ];

Of course then we ne the possibility to pass the histore to the assistant.

Sorry for such huge proposals to change the structure.

🇩🇪Germany jan kellermann

A first step could be to create a component for this issue queue. Maybe "ai_chatbot (DeepChat)"?

🇩🇪Germany jan kellermann

Hi Marcus, great! I tested your patch and extended the MR.
Unfortunately i had to add some "hacky" code also to reload the thread_id in deepchat-object.

Please review.

🇩🇪Germany jan kellermann

There is already an issues to review. Plese checkout this code and try: #3495565 Config for Drupal module Google Tag 2.x Active

If you want test manually, plese add

gtm.js
gtag.js
https://www.googletagmanager.com/ns.html

to the "Advaced > Sources" field and clear cache after this.

🇩🇪Germany jan kellermann

Updated the fork.

RTBC to the commits on modules/ai_assistant_api/src/AiAssistantApiRunner.php.

I dont know if the warning in modules/ai_assistant_api/src/Form/AiAssistantForm.php is neccessary.

🇩🇪Germany jan kellermann

PHPstan doesnt like the short hand version. Fixed to longer version.

Please review.

Production build 0.71.5 2024