Draft: Migrate Incidents Log to React

[kr-matthews]

This is maybe about half done? Possibly more? Hard to judge.

1. I'm not sure how pagination works, especially when combined with the search boxes. Is there a nice existing implementation to build into my WcaTable? I have it as part of the data passed in, but I don't really understand html attributes very well. I could create something from scratch easily enough with useState etc.
2. Probably similar to 1, but I'm not getting the table style to show up (alternating background for rows, etc), and padding around tags,
3. I'm not sure I'm handling links correctly; basically forming strings of html elements in Ruby and passing them into React.
4. I'm pretty sure my serializable_hash is either "wrong" or at least needs to be split up into some helper functions, but I'm not sure how best to proceed. In particular, I rewrote everything from WcaOnRails/app/helpers/incidents_helper.rb into WcaOnRails/app/models/incident.rb because I couldn't figure out any alternatives.

Any feedback/help is of course welcome.

Note: In the images, the 'after' has canViewDelegateMatters and such set to true temporarily, which is why additional columns are showing up.

Before:

After:

[kr-matthews]

It's been a long time but I've made some more progress over the last month or so. Originally, I was trying to avoid changing the look of the page at all, and have it essentially identical to the original page. But I think that's unnecessary, and makes things harder and less React-friendly, so I've moved away from that. I've mostly addressed the existing comments above (except there are some todos for handling links). I do have some new issues/questions though:

1. The popover tags for competitions and regulations have some issues. First, they don't work after you navigate via pagination. In incidents-log.js there's window.wca.reloadPopover(); which seems to resolve this, but I don't understand how to bring that into React. Second, they require passing in strings instead of JSX, so I don't see how to pass in a React function (such as for search). Is there a more React-friendly way to go about these tags? I saw Popover from reactstrap, not sure if it's easy/a good idea to add that to the project.
2. The original incidents log was using wca_table, which I initially translated into React. But maybe this isn't necessary, as we handle basic things like pagination and striped rows differently in React, and all the existing React tables just use Table directly.
3. The css for changing the entries-per-page isn't showing up. I don't fully understand where it was coming from in the original page. On a similar note, I probably need to clean up the table a little, for example to make the column widths consistent (they change when you change pages currently).
4. I haven't figured out how to implement the search functionality yet.

[gregorbg]

Originally, I was trying to avoid changing the look of the page at all, and have it essentially identical to the original page. But I think that's unnecessary, and makes things harder and less React-friendly, so I've moved away from that.

Agreed! Stay true to the original only as long as it makes sense. If pain(recreating_original) > pain(doing_it_the_react_way), then I don't mind redesigns that follow the React way of doing things. My primary objective is that the usage and usability "feels" the same for the user, so for example I wouldn't want to immensely restructure the entire table contents. But changing some design aspects in favor of (re-)using existing React components is totally fine!

About your questions: (Numbering follows the original message, didn't bother copying all of the questions so as to not blow up the length of this comment)

1. popover is the Bootstrap way of doing things. Our React UI framework of choice is semantic-ui, which has https://react.semantic-ui.com/modules/popup/. If you use them directly as React components and not via JS, I think you will also avoid having to reload anything with JS shenanigans.
2. Yes, you can just use Table from Semantic if it's more convenient (see also my remark about doing things "the React way" above.)
3. Yo no hablo CSS. But I guess you can just skip that feature for now and come back to it later.
4. My guess is that you would have to use https://react.semantic-ui.com/modules/search/ to filter which incidents get displayed in the first place, and then load those into the table as you do now.

[viroulep]

Originally, I was trying to avoid changing the look of the page at all, and have it essentially identical to the original page. But I think that's unnecessary, and makes things harder and less React-friendly, so I've moved away from that.

Please don't bother keeping the original design! Semantic UI has reactive tables so you can just go straight to the easy implementation and use them IMO.

Regarding point 4: feel free to take a look at how our omnisearch input is implemented here. It's totally overkill for that specific situation, but it gives you a rough idea about how the search input from Semantic UI works: you simply need some source of options (eg: a json api endpoint, some hardcoded json objects), with appropriate attributes, and it will "just work".

However it doesn't quite fit into how it's been implemented: right now all incidents are loaded, and filtering is done on all elements. One "smarter" way of doing that would be to forward the filtering data to the api endpoint so that it would return you only the incidents you need. My suggestion to move forward with the general "filtering" feature would be to:

• have a input "search" field from which we collect a string.
• have a multiple search selection filled with incidents tags you can select (similarly to what is done now, it's just a matter of feeding the multi search), from which we get a coma-separated list of tags.
• have an "index" json endpoint that paginates/filters the incidents (similarly to how the posts index is done here).
• feed a Semantic UI Table with the data from that endpoint

It does look like recreating the thing from scratch, but I do think this is the fastest/cleanest way to go about that (and besides the slight controller action to rewrite it's mostly just react work!).

[kr-matthews]

@gregorbg Thanks. For 1 I've got Popup working for the Tags, it's working great. For 2, I've removed the WcaTable component I originally had. I'll leave 3 (css issue) until the end as it's least important.

@viroulep Thanks for the clearly laid out suggestion for 4 - I think I understand and like the sound of it, and have started (slowly) working towards it.

[kr-matthews]

Regarding point 4: feel free to take a look at how our omnisearch input is implemented here. It's totally overkill for that specific situation, but it gives you a rough idea about how the search input from Semantic UI works: you simply need some source of options (eg: a json api endpoint, some hardcoded json objects), with appropriate attributes, and it will "just work".

I haven't used API endpoints before and I'm having trouble making the search "just work". I can provide the incidents via the controller, and paginate them, just like Posts are currently set up, providing whichever page of the pagination I want. And I can see how I would pass in more params and sort/filter the results in the controller. But I think the point of all this was to take advantage of the magic search (otherwise I'm just moving the filtering/sorting/pagination from React to Ruby) and that's where I'm stuck. For example, I can search competitions for clock at https://www.worldcubeassociation.org/api/v0/search/competitions?q=clock, but I can't seem to make q= work with posts at https://www.worldcubeassociation.org/posts.json?q=championship. Googling hasn't helped much.

[viroulep]

The API endpoint uses params[:q] and forward it to the Model.search method with any additional parameters given (you can actually search posts with that endpoint by the way).

Using https://www.worldcubeassociation.org/posts.json?q=championship you end up in the index action of the posts controller here, which doesn't take into account the q parameter.

But I think the point of all this was to take advantage of the magic search (otherwise I'm just moving the filtering/sorting/pagination from React to Ruby) and that's where I'm stuck.

This part I'm not sure what you mean, because it could be interpreted in a couple of way:

• you want incidents to be searchable in the "omnisearch" ("magic"?) box at the top right corner of the homepage
• you want an endpoint to which you can talk to and provide a search query
• you want to keep the filtering and search "actions" within react

IMO it was a mistake on my part to implement filtering and search in javascript in the first place. The main drawback of this approach is that it forces the client to load all incidents in js to be able to filter/search them. I believe it would be better for the React components to let the user select any kind of filter/search we want, then forward them to the server (along with the paging parameters), so that it only provides the necessary incidents to load.

If you did mean that you wanted incidents to be searchable through omnisearch, that's acutally a great idea I didn't think of, and it should just be a matter of adding a search method to the incident model (similar to what is done for users and competitions for instance), and adding an endpoint like one of these.

If you need some help with the "implement an API endpoint we can talk to by providing search query and params" I can definitely help you out, it's actually very straightforward to do!

[kr-matthews]

At this point, there are two main things left to do.

1. Make the string search and tag filter via the API work. @viroulep is helping with that (thanks!). The UI is already there and set up.
2. Some css issues: there are no gaps between tags, and the incidents-per-page selector area looks pretty bad. This should be easy to fix, I just don't like this stuff and so have been putting it off.

Old tags vs new tags:

Old selector vs new selector:

One thing I realized is that if you visit a particular incident, the old tags are still used, and have a link to search for an incident (see image below) - but that will no longer work. I think it's probably ok to leave those non-functional for now, and they'll naturally be fixed once that page is migrated to react (I can probably work on migrating the rest of the incidents log-related pages after this PR - they should be much easier/faster!).

Further, not quite related to this PR, but A1a2++++ seems to be an incorrect tag, there is no such guideline.

[kr-matthews]

I've got the search/filter working - thanks @viroulep! Bonus: the original text-search only searched titles, but now it will search the body and everything else too.

The minor css clean-up I mentioned earlier is still required, I'll work on that next.

There's an annoying quirk where quickly typing something into the search box (like "clock") briefly shows the correct results, then jumps back to showing all results. Probably something with the delay between sending the API request and getting the results back? I'll investigate more later.

It's currently not possible to sort the results; originally you could sort by "happened during" and "sent in digest". These don't seem that important, but I think I understand how to do that and can include that if we want.

And as mentioned in my previous comment: there's an issue with searching by tag from within individual incidents; and the incorrect A1a2++++ tag - who should I contact about that?

[viroulep]

And as mentioned in my previous comment: there's an issue with searching by tag from within individual incidents; and the incorrect A1a2++++ tag - who should I contact about that?

The tag itself is correct, but the guideline existed only until 2018: https://github.com/thewca/wca-regulations/blob/official-2018-01-01/wca-guidelines.md#-article-a-speed-solving

It doesn't display it as a real regulation/guideline number, but if I'm not mistaken it doesn't cause an issue when searching for its value right? If yes then I don't think there is anything more to do for that.

[kr-matthews]

I think I've addressed the minor css issues I mentioned earlier.

If yes then I don't think there is anything more to do for that.

Yes, that's the case, so I'll just leave it. Thanks.

For the non-functional search-by-tag when viewing an individual incident, maybe I should just remove that ability as part of this PR, and it can be re-added when that page is migrated to React?

Since rebasing, I've been getting this alert every time I reload the incidents log when running locally:

user: krmatthews
AVOID eager loading detected
  Incident => [:incident_tags]
  Remove from your query: .includes([:incident_tags])
Call stack
  [...]/worldcubeassociation.org/WcaOnRails/lib/middlewares/warden_user_logger.rb:13:in `call'
  [...]/worldcubeassociation.org/WcaOnRails/lib/middlewares/fix_accept_header.rb:14:in `call'

Similar alerts have always come up when I visit certain other pages, like 'my competitions'. Is this something to be concerned about? It's annoying at least.

When typing into the search box, every new character fires a new fetch, and when they're so close together they don't always return in the correct order: Would it be better to add some buffer of non-typing time before updating the url to fetch from, or to have a separate PR which updates the useLoadedData hook to ignore any out-dated fetch (assuming that's possible)? Or something else?

[gregorbg]

Similar alerts have always come up when I visit certain other pages, like 'my competitions'. Is this something to be concerned about? It's annoying at least.

In the WcaOnRails folder, create a file called .env.development.local and paste the following contents:

DISABLE_BULLET=true

(see .env.development to learn about other values you can override)

Would it be better to add some buffer of non-typing time before updating the url to fetch from [...]

Yes please! Modifying the useLoadedData hook globally seems too complicated right now.

[viroulep]

Would it be better to add some buffer of non-typing time before updating the url to fetch from, or to have a separate PR which updates the useLoadedData hook to ignore any out-dated fetch (assuming that's possible)? Or something else?

We already have a debounce hook to "buffer" a given value, you can check how it's used here; it should be very straightforward to use it in your component.

Similar alerts have always come up when I visit certain other pages, like 'my competitions'. Is this something to be concerned about? It's annoying at least.

It definitely depends on the context, but in general yes it's interesting to take a look at these alerts: it may indicate that we're running into a n+1 query or that we actually load data that we don't need. For instance let's assume we load registrations for a competition and then access each registration's user; if we just do Registration.where(competitionId: "something").map(&:user) it will fire one SQL query for all registrations, and one SQL query per user (!). Bullet makes sure to emit an alert when we do that, so that we can fix this to be Registration.includes(:user).where(competitionId: "something").map(&:user), where only two SQL queries will be fired: one for all registrations, one for all users for these registrations.

Of course sometimes it has barely any impact on the performance (eg: if you forget to include the teams when loading a user it's definitely fine, we just have a couple of teams max per user), but if you're manipulating a big chunk of data it's definitely worth activating bullet just to check nothing weird is going on.

It's definitely something you can turn off (like Gregor suggested) when you're not looking into how the perfs of your controller's action are doing.

[kr-matthews]

Thanks both for the feedback about the alerts.

We already have a debounce hook to "buffer" a given value, you can check how it's used here; it should be very straightforward to use it in your component.

Perfect, that was extremely easy to use.

I think I've done everything and it's ready to review (though I'm sure some tests will fail and I'll have to make minor updates). I would still like some feedback from @gregorbg on these two points:

It's currently not possible to sort the results; originally you could sort by "happened during" and "sent in digest". These don't seem that important, but I think I understand how to do that and can include that if we want.

For the non-functional search-by-tag when viewing an individual incident, maybe I should just remove that ability as part of this PR, and it can be re-added when that page is migrated to React?

[gregorbg]

It's currently not possible to sort the results; originally you could sort by "happened during" and "sent in digest". These don't seem that important, but I think I understand how to do that and can include that if we want.

Please estimate for yourself how much work this would entail. My priority is to get this PR merged to free you up for future tasks, but if it's just a three-minute two-line change then we might as well just go for it.

For the non-functional search-by-tag when viewing an individual incident, maybe I should just remove that ability as part of this PR, and it can be re-added when that page is migrated to React?

Agreed.

[kr-matthews]

It's currently not possible to sort the results; originally you could sort by "happened during" and "sent in digest". These don't seem that important, but I think I understand how to do that and can include that if we want.

Please estimate for yourself how much work this would entail. My priority is to get this PR merged to free you up for future tasks, but if it's just a three-minute two-line change then we might as well just go for it.

It's more than a few minutes/lines, but still relatively easy in theory. However, the sorting has to be done in index of incidents_controller.rb, and I can't seem to get that working with sort_by or order and digest_sent? or digest_sent_at (fetching returns an error). So I'm sure it's pretty easy, but I'm just stuck on the Ruby side of things.

For the non-functional search-by-tag when viewing an individual incident, maybe I should just remove that ability as part of this PR, and it can be re-added when that page is migrated to React?

Agreed.

Removed.

For the failing tests, I'm seeing HTML screenshots containing

  <div data-react-class="IncidentsLog" data-react-props="{&quot;canManageIncidents&quot;:null,&quot;canViewDelegateMatters&quot;:null,&quot;allTags&quot;:[&quot;1a&quot;,&quot;a&quot;,&quot;b&quot;,&quot;c&quot;]}" data-react-cache-id="IncidentsLog-0" id="incidents_log"></div>

and am not sure what the problem is so far.

[kr-matthews]

I've fixed some of the failing tests, and added the search string and filter tags to the url search params, so that you can now share links. This also means that the 'search by this tag' links on individual incidents work again, so I reverted the commit which removed those.

Just 4 failing tests now, which I'll look into next time.

Edit: The test to search by tag just visits the url - visit "/incidents?tags=misscramble" - but based on the html image generated by the test (rspec ./spec/features/incident_management_spec.rb:24) this doesn't cause 'misscramble' to populate the tag search box. But manually visiting the url does seem to work. So I'm not sure what's going on.

It also seems that rspec ./spec/features/incident_management_spec.rb:48 is passing locally for me, but GitHub says it's failing? Also not sure what's going on.

Is there a way to run the pre-commit hooks locally before pushing?

[kr-matthews]

One issue, when logged in as myself I can see the 'New Incident' button, which I shouldn't be able to see (and can't on the real site). For some reason the canManageIncidents prop from rails is coming in as true. I haven't been able to figure out what's going on, I think it was working before.

I also reorganized the files a bit.

~Edit: The failing tests seems unrelated to my changes?~ Fixed by merging with master.

[gregorbg]

One issue, when logged in as myself I can see the 'New Incident' button, which I shouldn't be able to see

Oh but of course you should! On local and staging systems, all WST members have full admin privileges:

def can_manage_incidents?
  admin? || wrc_team?
end

def admin?
  Rails.env.production? && EnvVars.WCA_LIVE_SITE? ? software_team_admin? : software_team?
end

The reason that you cannot see the button on prod is because there we limit admin privileges to WST Senior members only.