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

Setting embargo "change all files within" to true #4520

Merged
merged 1 commit into from
Sep 17, 2020

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Sep 15, 2020

This change now echoes the lease behavior rendering. See

<%= check_box_tag "leases[#{i}][copy_visibility]", curation_concern.id, true %>

for the similar view.

Closes #4161

Per discussions with the product owner and QA on Github and Slack:

Jeremy (Developer, Github)

I think there is some interactive behavior that needs to be captured,
as there are LOTs of check-boxes in play.

There is the "Select All Check Box", there's several "Work checkboxes"
and for each work checkbox, there's an embargo checkbox. When I look
at https://github.com/samvera/hyrax/pull/4319/files, it appears that
when someone clicks the work checkbox, it will click the associated
embargo checkbox.

However, what I'm understanding from QA is one of two things:

  1. When someone checks the "Select All Check Box" that it would
    cascade down to each work AND down to each embargo check boxes.
  2. All embargo checkboxes should start out checked, even though the
    work checkboxes do not start checked.

Please advise.

Crystal (QA, Slack)

When digging through the other ticket linked in the card, this ticket
was created to "create a ticket to make it so the checkbox to change
all files within is selected by default like Leases". SO I believe
the desired behavior is to have the select all automatically selected
on this tab. I have to jump in to some meetings currently, but will
be looking more into the functionality on Leases this aftternoon to
make sure that is the proper behavior. When I am finished I will
update the card and let you know. Also, if Julie comes back and says
she wants a different behavior obviously that overrides all of this.
I am just going off the ticket.

**Julie (PO, Slack)

Hi - I think what Crystal describes makes sense and I don’t want to
add on any additional changes to what this issue is
suggesting. Matching this up with Leases behavior is a good change and
if an item is selected to deactivate embargo then the “Change all
files within” checkbox should be auto-checked as well. If that makes
this act more like Leases and eases up the number of checkboxes that
have to be checked to to take the Deactivate Embargo action, then that
works. Does that help?

To test this in the UI, I cheated a bit. On the
app/views/hyrax/embargoes/_list_expired_active_embargoes.html.erb
partial I added the following code before line 1:

<%
embargo = Struct.new(:id, :embargo_release_date) do
  def human_readable_type
    "Human"
  end
  def visibility
    "private"
  end
  def visibility_after_embargo
    "public"
  end
end

assets_with_expired_embargoes = [embargo.new(1, 1.day.ago)]
%>

This allowed me to create a data structure that enabled UI rendering.

One thing I noticed is that in the Embargo UI there's a "Select All" yet
there isn't an equivalent in the Lease UI. It's an odd little
discrepency.

Note: To test the lease page, I added the following at the beginning of
the app/views/hyrax/leases/_list_expired_active_leases.html.erb partial:

<%
lease = Struct.new(:id) do
  def human_readable_type
    "Human"
  end
  def visibility
    "public"
  end
  def lease_expiration_date
    Date.today
  end
  def visibility_after_lease
    "private"
  end
end
assets_with_expired_leases = [lease.new(1)]
%>

@samvera/hyrax-code-reviewers

This change now echoes the lease behavior rendering.  See
https://github.com/samvera/hyrax/blob/b1c3624418621689927f56598220ccfaec5c7f4d/app/views/hyrax/leases/_list_expired_active_leases.html.erb#L43
for the similar view.

Closes #4161

Per discussions with the product owner and QA on Github and Slack:

**Jeremy (Developer, Github)**

> I think there is some interactive behavior that needs to be captured,
> as there are LOTs of check-boxes in play.
>
> There is the "Select All Check Box", there's several "Work checkboxes"
> and for each work checkbox, there's an embargo checkbox. When I look
> at https://github.com/samvera/hyrax/pull/4319/files, it appears that
> when someone clicks the work checkbox, it will click the associated
> embargo checkbox.
>
> However, what I'm understanding from QA is one of two things:
>
> 1. When someone checks the "Select All Check Box" that it would
>    cascade down to each work AND down to each embargo check boxes.
> 2. All embargo checkboxes should start out checked, even though the
>   work checkboxes do not start checked.
>
> Please advise.

**Crystal (QA, Slack)**

> When digging through the other ticket linked in the card, this ticket
> was created to "create a ticket to make it so the checkbox to change
> all files within is selected by default like Leases".  SO I believe
> the desired behavior is to have the select all automatically selected
> on this tab.  I have to jump in to some meetings currently, but will
> be looking more into the functionality on Leases this aftternoon to
> make sure that is the proper behavior.  When I am finished I will
> update the card and let you know.  Also, if Julie comes back and says
> she wants a different behavior obviously that overrides all of this.
> I am just going off the ticket.

**Julie (PO, Slack)

> Hi - I think what Crystal describes makes sense and I don’t want to
> add on any additional changes to what this issue is
> suggesting. Matching this up with Leases behavior is a good change and
> if an item is selected to deactivate embargo then the “Change all
> files within” checkbox should be auto-checked as well. If that makes
> this act more like Leases and eases up the number of checkboxes that
> have to be checked to to take the Deactivate Embargo action, then that
> works. Does that help?

To test this in the UI, I cheated a bit.  On the
`app/views/hyrax/embargoes/_list_expired_active_embargoes.html.erb`
partial I added the following code before line 1:

```ruby
<%
embargo = Struct.new(:id, :embargo_release_date) do
  def human_readable_type
    "Human"
  end
  def visibility
    "private"
  end
  def visibility_after_embargo
    "public"
  end
end

assets_with_expired_embargoes = [embargo.new(1, 1.day.ago)]
%>
```

This allowed me to create a data structure that enabled UI rendering.

One thing I noticed is that in the Embargo UI there's a "Select All" yet
there isn't an equivalent in the Lease UI.  It's an odd little
discrepency.

Note: To test the lease page, I added the following at the beginning of
the `app/views/hyrax/leases/_list_expired_active_leases.html.erb` partial:

```ruby
<%
lease = Struct.new(:id) do
  def human_readable_type
    "Human"
  end
  def visibility
    "public"
  end
  def lease_expiration_date
    Date.today
  end
  def visibility_after_lease
    "private"
  end
end
assets_with_expired_leases = [lease.new(1)]
%>
```
@jeremyf
Copy link
Contributor Author

jeremyf commented Sep 15, 2020

This is the screenshot for Embargo after the change (on a "fresh page").
Screen Shot 2020-09-15 at 12 55 49 PM

This is the screenshot for Lease without any changes the change (on a "fresh page").
Screen Shot 2020-09-15 at 12 55 58 PM

@no-reply no-reply merged commit 35b5895 into master Sep 17, 2020
@no-reply no-reply deleted the gh-4161-default-behavior-for-files branch September 17, 2020 16:47
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.

Default behavior for files when disabling selected embargoes
2 participants