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.

Sign up for GitHub

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

Jump to bottom

Default behavior for files when disabling selected embargoes #4161

Closed
dlim87 opened this issue Nov 11, 2019 · 6 comments · Fixed by #4319 or #4520
Closed

Default behavior for files when disabling selected embargoes #4161

dlim87 opened this issue Nov 11, 2019 · 6 comments · Fixed by #4319 or #4520
Assignees
@jeremyf
Labels
enhancement needs rework Story not complete and needs further action
Milestone
3.x series

Comments

@dlim87
Copy link

dlim87 commented Nov 11, 2019

Descriptive summary

As per #3993, by default Change all files within... in Dashboard > Manage embargos > Expired active embargos should be checked.

Rationale

Defaulting to select the files is the behavior for the similar form in leases. It would make more sense for the default action to also disable the embargo on the items inside a work while removing it for the work. The current default behavior just changes the work's visibility, while leaving the items inside it embargoed

Expected behavior

by default Change all files within... in Dashboard > Manage embargos > Expired active embargos should be checked.

Actual behavior

by default Change all files within... in Dashboard > Manage embargos > Expired active embargos are not checked.
Image 2019-11-11 at 2 27 16 PM

Steps to reproduce the behavior

  1. Log as a user who has some expired embargoes they can manage
  2. go to the dashboard
  3. click on manage embargoes
  4. click on expired active embargoes

Related work

#3993 #4002
@dlim87 dlim87 mentioned this issue Nov 11, 2019
Closed
@dlim87
Copy link
Author

dlim87 commented Nov 11, 2019

@samvera/hyrax-repo-managers

@no-reply no-reply added this to the 3.x series milestone Nov 12, 2019
@blancoj blancoj self-assigned this Dec 4, 2019
@blancoj blancoj mentioned this issue Dec 4, 2019
Closed
@blancoj blancoj mentioned this issue May 19, 2020
Merged
blancoj added a commit that referenced this issue May 21, 2020
@blancoj
set checkbox for files within. Fixes issue #4161
b034cff
no-reply pushed a commit that referenced this issue May 21, 2020
@blancoj
set checkbox for files within. Fixes issue #4161
a2ffd99
@MariaStarPower
Copy link

The embargoes in the Expired Active Embargoes tab are not selected by default:

Image 2020-09-09 at 5 03 35 PM

@crisr15
Copy link

crisr15 commented Sep 11, 2020

Embargos should be selected by default but are not currently.

@crisr15 crisr15 added the needs rework Story not complete and needs further action label Sep 11, 2020
@jeremyf jeremyf assigned jeremyf and unassigned blancoj Sep 14, 2020
@jeremyf
Copy link
Contributor

jeremyf commented Sep 14, 2020

@crisr15 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.

@jeremyf jeremyf reopened this Sep 14, 2020
jeremyf added a commit that referenced this issue Sep 15, 2020
@jeremyf
Setting embargo "change all files within" to true
0e93621 
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 jeremyf mentioned this issue Sep 15, 2020
Merged
@jeremyf
Copy link
Contributor

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 pushed a commit that referenced this issue Sep 17, 2020
@jeremyf
Setting embargo "change all files within" to true
35b5895 
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)]
%>
```
@MariaStarPower
Copy link

Pass - all of the Expired Active Embargoes are selected by default.

Image 2020-10-08 at 11 56 10 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jeremyf jeremyf

Labels
enhancement needs rework Story not complete and needs further action
Projects
None yet
Milestone
3.x series
6 participants
@jeremyf @no-reply @dlim87 @blancoj @crisr15 @MariaStarPower