@diosmosis opened this Pull Request on November 11th 2021 Member

Description:

This PR is based off of https://github.com/matomo-org/matomo/pull/18225.

This PR has a breaking change to createAngularAdapter() so other Vue PRs in progress may need to be modified (unless they are merged before this, then I'll fix the conflicts here).

Changes:

  • Add typescript types for mousetrap library.
  • deprecate autocomplete-matched as it's trivial to implement in siteselector and is not used anywhere else.
  • in broadcast.propagateNewPage() add extra parameter for supplying the entire new URL to load. in this case, it does not build the URL using other parameters.
  • add a MatomoUrl.updateUrl() method that reloads the entire URL (search + hash)
  • make sure promise returned by AjaxHelper.fetch has an abort() method
  • in createAngularAdapter() add vue instance to the event handler functions and postCreate(). this is so ngModel could be connected to v-model in Vue.
  • migrated the siteselector directive.
  • migrated the quick-access directive.
  • migrated the siteselector-model service as the SitesStore. This is shared between quick-access and siteselector. The only state that's shared is the initial list of sites that is loaded once, unlike currently where the list of currently displayed sites is also shared between the two directives.
  • in createAngularAdapter() allow specifying a require: field to the angularjs directive that is created.
  • add the ability to transform values when binding from angularjs => vue (for example, converting values to booleans, as in noclear for siteselector)
  • disable webpack asset size hints (since our assets are already bigger at the end than what is recommended by webpack).
  • when changing sites, removed the showAjaxLoading = false parameter so MatomoUrl.updateUrl() will stay simple. Doesn't seem to have much of an effect IMO so I think it's ok.

image

Review

@diosmosis commented on November 12th 2021 Member

Explanation for UsersManager screenshot updates:

The users manager root component gets set an initial site ID. Since there is not idSite in the URL in the test, it is set to 1. The site w/ ID = 1 is SLEEP, which is correct now. Before it seems it wasn't being initialized correctly in the site selector.

@peterhashair commented on November 15th 2021 Contributor

@diosmosis also I got an question for function naming.
like that one, do we need a camel case?

export { default as Menudropdown } from './Menudropdown/Menudropdown.vue';
@diosmosis commented on November 15th 2021 Member

I got an question for function naming. like that one, do we need a camel case?

Yes, that would probably be better :+1:

@diosmosis commented on November 16th 2021 Member

converted to draft so it won't get merged until after the 4.6.0 release

@justinvelluppillai commented on November 17th 2021 Contributor

converted to draft so it won't get merged until after the 4.6.0 release

PRs merged to 4.x-dev won't be released in 4.6.0

@diosmosis commented on November 17th 2021 Member

@justinvelluppillai ok, removing draft status

@sgiehl commented on November 17th 2021 Member

@diosmosis I'm currently doing some local testing of the changes. Will leave some more comments when I find something else.

  • [x] It seems the search results in site selector are not escaped correctly. I have a website with the name <a href="#">?</a>, it displays correctly when listed on opening the siteselector. But when searching, it is only displayed as ?, the html tags seems to be inserted. So maybe a script tag might evaluate here as well.

    Note: In quick search that seems to work correctly.

  • [x] When I type something into the search box in site selector and then click on the input field again, the field either looses it's content (if the request is still pending), or it sometimes even disappears (when the request had finished)

  • [x] Pressing the f key highlights the quick search visually, but doesn't give the input box the focus anymore

  • [x] Pressing the w key opens and highlights the site selector visually, but doesn't give the input box the focus anymore

  • [x] Unrelated to the changes here. But pressing d doesn't open the date picker any longer. Maybe that's due to other vue changes before
@diosmosis commented on November 17th 2021 Member

So maybe a script tag might evaluate here as well.

It's sanitized so dangerous HTML is removed. The other html I guess isn't being escaped properly.

@diosmosis commented on November 17th 2021 Member

It seems the search results in site selector are not escaped correctly. I have a website with the name ?, it displays correctly when listed on opening the siteselector. But when searching, it is only displayed as ?, the html tags seems to be inserted. So maybe a script tag might evaluate here as well.

Can you check again? this is working for me:

image

When I type something into the search box in site selector and then click on the input field again, the field either looses it's content (if the request is still pending), or it sometimes even disappears (when the request had finished)

Looks like there's a bug in the original code. Should be fixed in the vue PR now.

Pressing the f key highlights the quick search visually, but doesn't give the input box the focus anymore

Fixed.

Pressing the w key opens and highlights the site selector visually, but doesn't give the input box the focus anymore

Fixed.

Unrelated to the changes here. But pressing d doesn't open the date picker any longer. Maybe that's due to other vue changes before

Found the issue, will fix in PR to next_release.

@sgiehl commented on November 18th 2021 Member

@diosmosis hm. funny. It works when searching for ? for me as well. But when searching for any letter that actually isn't in the site's name, it doesn't.

image

@diosmosis commented on November 18th 2021 Member

@sgiehl should be fixed. Looks like the issue only occurred when searching for html entity letters (eg, searching for gt which in the backend will match &lt;img onsrc...).

This Pull Request was closed on November 19th 2021
Powered by GitHub Issue Mirror