Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "actions" for labels for users who can't do actions on labels #5289

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

BWindey
Copy link
Contributor

@BWindey BWindey commented Jan 13, 2024

This pull request removes the actions-column entirely for users who don't have the right to perform actions on labels.

For students

Pc normal window

Before:
image
After:
image

Small window

(simulated by making window as small as possible and zooming in to 130%)

Before: After:
image image

For admins (Zeus)

Pc normal window

Before:
image
After:
image

Small window

(simulated by making window as small as possible and zooming in to 130%)

Before: After:
image image

@BWindey BWindey requested a review from a team as a code owner January 13, 2024 17:24
@BWindey BWindey requested review from jorg-vr and chvp and removed request for a team January 13, 2024 17:24
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Just a small remark on your code.

app/views/labels/_labels_table.html.erb Show resolved Hide resolved
Copy link
Contributor

@jorg-vr jorg-vr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

Could you put the latest images in the pr description?

Also did you test this on smaller screen widths? The 9-3 vs 10-2 discussion might be more relevant in those cases

@jorg-vr jorg-vr added enhancement A change that isn't substantial enough to be called a feature bug Something isn't working and removed enhancement A change that isn't substantial enough to be called a feature labels Jan 15, 2024
@BWindey
Copy link
Contributor Author

BWindey commented Jan 15, 2024

Updated the original PR to include the images

@bmesuere bmesuere added deploy mestra Request a deployment on mestra and removed deploy mestra Request a deployment on mestra labels Jan 16, 2024
<th><%= t "labels.index.actions" %></th>
<th class="text-end"><%= t "labels.index.exercises" %></th>
<% if policy(Label).edit? %>
<th class="text-end"><%= t "labels.index.actions" %></th>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<th class="text-end"><%= t "labels.index.actions" %></th>
<th class="text-end"></th>

The label is redundant, so I would remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would remove the "Actions" title, correct?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, then we could remove the conditional entirely for the tabel header.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it would remove the Actions label. Removing the conditional for the table header is not a good idea. You should have the same number of header columns as body columns.

Copy link
Contributor Author

@BWindey BWindey Jan 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems logical, ok.

I just noticed something too. There are always 3 body columns, as the conditional is seperated into permission to edit and permission to destroy. Both checks happen inside the column tag.

How does that work with there only being 2 columns defined (for users without permission), but 3 actual columns, the last one empty?

...
  <colgroup>
      <% if policy(Label).edit? %>
        <col class="col-6"/>
        <col class="col-3"/>
        <col class="col-3"/>
      <% else %>
        <col class="col-10"/>
        <col class="col-2"/>
      <% end %>
    </colgroup>
  
  ...
  <tbody>
    <% labels.each do |label| %>
      <tr>
        <td><%= link_to label.name, label_path(label) %></td>
        <td class="text-end"><%= link_to label.activities.count, activities_path({"labels[]" => label.name}) %></td>
        <td class="text-end">
          <% if policy(Label).edit? %>
            <%= link_to edit_label_path(label), class: "btn btn-icon" do %>
              <i class="mdi mdi-pencil"></i>
            <% end %>
          <% end %>
          <% if policy(Label).destroy? %>
            <%= link_to label, method: :delete, data: {confirm: t("general.are_you_sure")}, class: "btn btn-icon btn-icon-filled d-btn-danger" do %>
              <i class="mdi mdi-delete"></i>
            <% end %>
          <% end %>
        </td>
      </tr>
    <% end %>
  </tbody>

...

So the question is: are there users who have permissions to edit but not destroy, or vice versa?

  • If no, then we could move the tag inside the conditional and combine the two conditionals in one.
  • If yes, do we create an extra conditional around the tag with an 'or', or do we leave it like this? (And always have the third (empty) header to have an equal amount? Does the tag need an extra empty column then, or is that not possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditions of which users has which permission are handled in a separate file.
This file should not know how these conditionals are related.

Safest way would thus to simply write the check with an or around the <td> and use the same or around the column definitions

@jorg-vr jorg-vr requested a review from chvp January 16, 2024 12:45
@jorg-vr jorg-vr merged commit 97ae3bc into dodona-edu:main Jan 16, 2024
13 checks passed
@BWindey BWindey deleted the update-labels-page branch January 16, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants