Skip to content

Conversation

@josemigallas
Copy link
Contributor

@josemigallas josemigallas self-assigned this Jan 16, 2025
@josemigallas josemigallas force-pushed the THREESCALE-9875_account_users_index branch 2 times, most recently from 05d9f63 to 7afa5f5 Compare January 20, 2025 07:34
@josemigallas josemigallas requested a review from mayorova January 20, 2025 11:53
@mayorova
Copy link
Contributor

The sorting feature has some issues... I didn't dig for why, so just listing the errors:

  1. The first column is titled "Name", but sorting apparently happens on the username column.
  2. When trying to put some other colum in the "sort" query arg (e.g. name), it fails with
    Screenshot from 2025-01-20 17-06-45
  3. When trying to put something random in the direction query param, it fails with another error:
    Screenshot from 2025-01-20 17-07-10

So, it looks as if those query params are being passed to ActiveRecord directly?..
The same thing doesn't happen on some other pages (e.g. Accounts listing).

@mayorova
Copy link
Contributor

By the way, the trash icon doesn't match some existing ones, but I guess that's fine, because the other pages are probably using something obsolete...
image

@josemigallas
Copy link
Contributor Author

josemigallas commented Jan 21, 2025

TL;DR: I'm reverting the sorting feature of this page, it wasn't there in the first place (nobody asked for it) and it turns to be too time-consuming.

It can always be added later.

The sorting feature has some issues... I didn't dig for why, so just listing the errors:

  1. The first column is titled "Name", but sorting apparently happens on the username column.

Yes, that's on purpose. The "name" comes from a decorator method that returns "first_name + last_name OR username". Sorting by this method means all records will be loaded into memory and it's not good.

  1. When trying to put some other colum in the "sort" query arg (e.g. name), it fails with
    [...]
  2. When trying to put something random in the direction query param, it fails with another error:
    [...]

So, it looks as if those query params are being passed to ActiveRecord directly?.. The same thing doesn't happen on some other pages (e.g. Accounts listing).

That's how it's supposed to work. In Accounts listing, sorting is not implemented properly (due an early version of me not understanding the differences between order and sort_by.

The user is not supposed to modify the URL manually. You can also try passing per_page and page params and it will "break" the tables. As long as this is purely visual (Rails will take care of any vulnerabilities) I don't think it's worth the effort of validating this kind stuff.

@josemigallas josemigallas force-pushed the THREESCALE-9875_account_users_index branch from b553c9b to ea77c12 Compare January 21, 2025 08:56
@mayorova
Copy link
Contributor

That's how it's supposed to work. In Accounts listing, sorting is not implemented properly (due an early version of me not understanding the differences between order and sort_by.

The user is not supposed to modify the URL manually. You can also try passing per_page and page params and it will "break" the tables. As long as this is purely visual (Rails will take care of any vulnerabilities) I don't think it's worth the effort of validating this kind stuff.

Well, I have to disagree with this. If Rails protects from "breaking" the DB if the user puts some invalid stuff in the URL - OK, that's something. But I think it's not good. If an incorrect user input results in a 500 error (and a reported exception) - it's bad UX, IMO. The user for example can copy-paste the URL to another tab (and miss or add some characters accidentally in the process), and receiving an Internal Server Error because of this is not nice.

@josemigallas
Copy link
Contributor Author

That's how it's supposed to work. In Accounts listing, sorting is not implemented properly (due an early version of me not understanding the differences between order and sort_by.
The user is not supposed to modify the URL manually. You can also try passing per_page and page params and it will "break" the tables. As long as this is purely visual (Rails will take care of any vulnerabilities) I don't think it's worth the effort of validating this kind stuff.

Well, I have to disagree with this. If Rails protects from "breaking" the DB if the user puts some invalid stuff in the URL - OK, that's something. But I think it's not good. If an incorrect user input results in a 500 error (and a reported exception) - it's bad UX, IMO. The user for example can copy-paste the URL to another tab (and miss or add some characters accidentally in the process), and receiving an Internal Server Error because of this is not nice.

It's no ideal, it's just a compromise. A user can also tamper with the DOM, remove disabled buttons, submit forms with missing fields, etc. And in most cases it will break the page (because rails or formtastic or whatever framework doesn't take care of that). AFAIK the URL is out of the scope of UX per se, and frontend development don't "have to" take care of that. It's the UI and it's elements what offers and limits the functionality.

But I agree with you, it's not good and it shouldn't happen. But it's how rails work and validating url param manually isn't good either.

The best solution right now: don't add sorting columns! 😄

@josemigallas josemigallas force-pushed the THREESCALE-9875_account_users_index branch from ea77c12 to 9795577 Compare January 21, 2025 12:26
@josemigallas josemigallas merged commit 33882c0 into master Jan 22, 2025
19 of 23 checks passed
@josemigallas josemigallas deleted the THREESCALE-9875_account_users_index branch January 22, 2025 07:30
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.

3 participants