[PATCH v2 5/5] patch-list: add inline dropdown for delegate and state one-off changes

Raxel Gutierrez raxel at google.com
Wed Aug 4 04:30:58 AEST 2021


Sorry forgot to cc the list for the reply below; apologies for the
noise for those who already received this
-------------------
Hi,

Thanks for the review! :)

> This is pretty cool. My initial thoughts are as follows:
>
> Can we make these only appear editable if you are either:
>  - logged in,
>  - or ideally, if you have the rights to change them
>
> Currently I see a bunch of drop downs when I access the page as a
> non-logged-in user where I couldn't possibly change the state or
> delegate.

I tested the ideal case where users can only see the dropdown if they
have rights to change it. The loading of the page is significantly
slower as the permissions for each patch need to be checked. I also
tested the logged in state variant and it doesn't affect the loading
of the page significantly. Nonetheless, I think that the error
messages that are rendered at the top of the page are good enough for
those specific scenarios.

> I'm umming and ahing about whether I want to gate this feature entirely
> behind a user-specific setting: check out commit 6e32965b04cc ("'mpe
> mode': click to copy patch IDs") for an example. Or whether it should be
> on by default but disable-able via a setting. I'm a bit worried that it
> makes rows wider and our power users have tended to get a bit irritated
> when we do things that break their muscle memory.

I can't speak for power users but I feel the change in the horizontal
space is not very significant.

> Also, if there's an error setting a value (e.g. I'm not logged in) it
> would be nice if the value reset so that I only see what the actual
> state in the db.

Since not logged in users now can't see the dropdowns, this case is
covered. Nonetheless, I changed the functionality so that when
requests fail, the dropdown selection reverts to its previous
selection. The requests fails for unauthorized users making changes to
the patches, so it will cover the case of logged in users without
permissions as well.

Best,
Raxel


More information about the Patchwork mailing list