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

♻️ Favor existing method call over custom #6705

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Feb 20, 2024

This is technically a change in functionality, but one that moves
towards consistent method usage.

Prior to this commit, we were saying can_create_any_work? and
referencing the Hyrax.config.curation_concerns. Other places in the
code instead reference curation_concerns_models.

The difference is that curation_concerns_models includes FileSets and
Collections. Further, the odds that the user cannot create a Pcdm::Work
but could create a Collection or FileSet are low; meaning that this
simplification helps reduce logic foibles.

Also, as we look to Hyku, we are approaching curation concern
registration differently. The original model (e.g. GenericWork and
Image) are registered and we indicate the Valkyrie::Resources as
migrations of those model types. Meaning that
Hyrax.config.curation_concerns only has GenericWork but we want
these permissions to apply to the migrations as well.

If we don't approve this change, I'll revise Hyku's approach.

Below is a quick list of the app usages of the method:

Using rg can_create_any_work app

`app/views/hyrax/my/works/index.html.erb`
18:  <% if current_ability.can_create_any_work? %>

This logic indicates if they can see the create a new work type or not.

`
36:  <% if current_ability.can_create_any_work? && Hyrax.config.analytics? %>

Do we show the user analytics information? Again the information should
only be of things they can create. So we aren't over-exposing any
information.

app/models/concerns/hyrax/ability.rb
106:    def can_create_any_work?
`app/presenters/hyrax/homepage_presenter.rb`

Below is the impact

def display_share_button?
  (user_unregistered? && Hyrax.config.display_share_button_when_not_logged_in?) ||
    current_ability.can_create_any_work?
end

This will either show or not show a work for sharing. They would still
need to be able to read the work to see the button.

This is technically a change in functionality, but one that moves
towards consistent method usage.

Prior to this commit, we were saying `can_create_any_work?` and
referencing the `Hyrax.config.curation_concerns`.  Other places in the
code instead reference `curation_concerns_models`.

The difference is that `curation_concerns_models` includes FileSets and
Collections.  Further, the odds that the user cannot create a Pcdm::Work
but could create a Collection or FileSet are low; meaning that this
simplification helps reduce logic foibles.

Also, as we look to Hyku, we are approaching curation concern
registration differently.  The original model (e.g. `GenericWork` and
`Image`) are registered and we indicate the Valkyrie::Resources as
migrations of those model types.  Meaning that
`Hyrax.config.curation_concerns` only has `GenericWork` but we want
these permissions to apply to the migrations as well.

If we don't approve this change, I'll revise Hyku's approach.

Below is a quick list of the app usages of the method:

Using `rg can_create_any_work app`

<details><summary>`app/views/hyrax/my/works/index.html.erb`</summary>

```
18:  <% if current_ability.can_create_any_work? %>
```

This logic indicates if they can see the create a new work type or not.

</details>

```
<details><summary>`<app/views/hyrax/dashboard/sidebar/_activity.html.erb`</summary>

```
36:  <% if current_ability.can_create_any_work? && Hyrax.config.analytics? %>
```

Do we show the user analytics information?  Again the information should
only be of things they can create.  So we aren't over-exposing any
information.
</details>

```
app/models/concerns/hyrax/ability.rb
106:    def can_create_any_work?
```

<details><summary>`app/presenters/hyrax/homepage_presenter.rb`</summary>

Below is the impact

```
def display_share_button?
  (user_unregistered? && Hyrax.config.display_share_button_when_not_logged_in?) ||
    current_ability.can_create_any_work?
end
```

This will either show or not show a work for sharing.  They would still
need to be able to read the work to see the button.
</details>
Copy link
Contributor

@dlpierce dlpierce left a comment

Choose a reason for hiding this comment

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

This is good. When I see curation_concerns I think of the registered work models, but not collections, filesets, etc.

@dlpierce dlpierce merged commit 23d2d05 into main Feb 20, 2024
6 checks passed
@dlpierce dlpierce deleted the favor-method-call branch February 20, 2024 16:19
@dlpierce dlpierce added the notes-minor Release Notes: Non-breaking features label Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-minor Release Notes: Non-breaking features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants