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

Depositing a work with past embargo gives no id error #4833

Closed
rjkati opened this issue Mar 25, 2021 · 8 comments · Fixed by #4885
Closed

Depositing a work with past embargo gives no id error #4833

rjkati opened this issue Mar 25, 2021 · 8 comments · Fixed by #4885
Assignees
Labels

Comments

@rjkati
Copy link

rjkati commented Mar 25, 2021

Descriptive summary

Depositing a work with an embargo date in the past gives an "No ID provided for ActiveFedora::Base" error.
noID

https://nurax-dev.curationexperts.com/concern/generic_works?locale=en

Expected behavior

Depositing a work with a past embargo should fail to be created and entered information should be preserved (per #3978 )

Actual behavior

The user receives an error message.

Steps to reproduce the behavior

  1. Log in
  2. Create a work
  3. Set an embargo or lease date to before today
  4. Press the save button

Related work

Link to related tickets or prior related work here.

@no-reply
Copy link
Contributor

@jeremyf i wonder if you could take a look at this one? i noticed that the new EditPermissionsService is implicated.

@no-reply no-reply added the bug label Mar 25, 2021
jeremyf added a commit that referenced this issue Apr 23, 2021
This commit serves two purposes:

1) First, and most important, it fixes #4833
2) Second, it unifies the logic for determining the ids

More on 4833 (in case you don't have access to Github)

The observed error is as follows:

> Depositing a work with past embargo gives no id error
>
> Depositing a work with an embargo date in the past gives an "No ID
> provided for ActiveFedora::Base" error.
>
> Expected Behavior: Depositing a work with a past embargo should fail
> to be created and entered information should be preserved

In debugging the application, the `@object.member_of_collection_ids` was
the following: `["", "admin_set/default"]`

That appears to come from the form submitting a select box that has no
selected value (e.g., the `""` value).  I verified this logic change by
UI interaction.
jeremyf added a commit that referenced this issue Apr 23, 2021
This documentation reflects some of what I worked through when testing
a bug fix for #4833.  My intention is to continue working on this code,
but only after a consultation.
jeremyf added a commit that referenced this issue Apr 26, 2021
Prior to this commit, we were not verifying that a user's input for a
release_date was in fact a parseable date.  Chrome and Firefox apply a
date picker "widget" that enforces entries for a `type="date"`.
However, Safari does not.

Closes #4757

Note, to test this in the UI requires a fix for #4833.  That fix, at the
time of crafting this commit message, can be seen at PR #4885.
jeremyf added a commit that referenced this issue Apr 26, 2021
Prior to this commit, we were not verifying that a user's input for a
release_date was in fact a parseable date.  Chrome and Firefox apply a
date picker "widget" that enforces entries for a `type="date"`.
However, Safari does not.

Closes #4757

Note, to test this in the UI requires a fix for #4833.  That fix, at the
time of crafting this commit message, can be seen at PR #4885.
dlpierce added a commit that referenced this issue Apr 26, 2021
…past-embargo-gives-no-id-error

Gh 4833 depositing a work with past embargo gives no id error
jeremyf added a commit that referenced this issue Apr 26, 2021
This commit serves two purposes:

1) First, and most important, it fixes #4833
2) Second, it unifies the logic for determining the ids

More on 4833 (in case you don't have access to Github)

The observed error is as follows:

> Depositing a work with past embargo gives no id error
>
> Depositing a work with an embargo date in the past gives an "No ID
> provided for ActiveFedora::Base" error.
>
> Expected Behavior: Depositing a work with a past embargo should fail
> to be created and entered information should be preserved

In debugging the application, the `@object.member_of_collection_ids` was
the following: `["", "admin_set/default"]`

That appears to come from the form submitting a select box that has no
selected value (e.g., the `""` value).  I verified this logic change by
UI interaction.
jeremyf added a commit that referenced this issue Apr 26, 2021
This documentation reflects some of what I worked through when testing
a bug fix for #4833.  My intention is to continue working on this code,
but only after a consultation.
jeremyf added a commit that referenced this issue Apr 26, 2021
Prior to this commit, we were not verifying that a user's input for a
release_date was in fact a parseable date.  Chrome and Firefox apply a
date picker "widget" that enforces entries for a `type="date"`.
However, Safari does not.

Closes #4757

Note, to test this in the UI requires a fix for #4833.  That fix, at the
time of crafting this commit message, can be seen at PR #4885.
@crisr15
Copy link

crisr15 commented Apr 29, 2021

Tested in Nurax with a work that had a passed embargo date.

received error:
FireShot Capture 009 - Action Controller_ Exception caught - nurax-dev.curationexperts.com.pdf

@crisr15 crisr15 reopened this Apr 29, 2021
@jeremyf
Copy link
Contributor

jeremyf commented Apr 29, 2021

Looking at Nurax as of 2021-04-29, the SHA is b034218. And based on footer, Nurax tracks to the master branch, which we've since renamed to main.

Which has the following permission logic:

def object_unauthorized_collection_ids
@object_unauthorized_collection_ids ||= begin
limited_access = []
unauthorized_collection_ids = object_member_of - object_managed_collection_ids
if unauthorized_collection_ids.any?
unauthorized_collection_ids.each do |id|
# TODO: Can we instead use a SOLR query? This seems to be somewhat expensive. However, as this is
# used in administration instead of user front-end displays, I'm not as concerned.
collection = ActiveFedora::Base.find(id)
limited_access << id if (collection.instance_of? AdminSet) || collection.share_applies_to_new_works?
end
end
limited_access
end
end

My changes were from ab6c756

Which look as follows:

def object_unauthorized_collection_ids
@object_unauthorized_collection_ids ||= begin
unauthorized_collection_ids = object_member_of_ids - object_managed_collection_ids
qualified_resources = Hyrax.query_service.find_many_by_ids(ids: unauthorized_collection_ids).select do |resource|
qualifies_as_unauthorized_collection?(resource: resource)
end
qualified_resources.map { |resource| resource.id.to_s }
end
end

Below is the log, at the end of the log is the commit that Nurax is on. At the beginning of the log are all of the commits that are in the main branch but are not deployed to Nurax.

*   ab6c75616 — Merge pull request #4885 from samvera/gh-4833-depositing-a-work-with-past-embargo-gives-no-id-error Daniel Pierce, (2021-04-26) (HEAD -> main, origin/main)
|\  
| * 35af65db1 — Refactoring to extract inner block logic Jeremy Friesen, (2021-04-24)
| * 22c888138 — Factoring from ActiveFedora::Base to query_service Jeremy Friesen, (2021-04-24)
| * d2ddfd793 — Renaming local variable for clarity Jeremy Friesen, (2021-04-24)
| * 657ef7c83 — Documenting spot to remove ActiveFedora::Base call Jeremy Friesen, (2021-04-23)
| * 696da5dbb — Removing redundant `.any?` check Jeremy Friesen, (2021-04-23)
| * c97c31fc6 — Compacting array building logic Jeremy Friesen, (2021-04-23)
| * 76c68fb1f — Renaming method to better reflect return value Jeremy Friesen, (2021-04-23)
* |   17e3da911 — Merge pull request #4891 from samvera/solr-configset Jeremy Friesen, (2021-04-26)
|\ \  
| * | 91201a0d2 — give useful output when the Solr ConfigSet can't upload with auth tamsin johnson, (2021-04-26)
|/ /  
* | 6f5af1efd — Make dassie sidekiq depend on services Daniel Pierce, (2021-04-22)
* | c18846c04 — Adding TROUBLESHOOTING section for CONTAINERS.md Jeremy Friesen, (2021-04-22)
* | e810cba17 — chart: readd support for kubernetes versions before 1.19 tamsin johnson, (2021-04-22)
|/  
* 52b2934c6 — allow the SolrCloud assign-configset utility to create collections tamsin johnson, (2021-04-21)
* b034218b8 — chart: populate secrets and env when Minio is used tamsin johnson, (2021-04-20)

@crisr15
Copy link

crisr15 commented Apr 30, 2021

Nurax dev is not pulling from main. Testing is on hold until new code is deployed.

@jlhardes
Copy link
Contributor

I can verify this is working on a local hyrax app so once Nurax-dev is pulling from main again we can verify there and close this issue.

jeremyf added a commit that referenced this issue May 4, 2021
Prior to this commit, we were not verifying that a user's input for a
release_date was in fact a parseable date.  Chrome and Firefox apply a
date picker "widget" that enforces entries for a `type="date"`.
However, Safari does not.

Closes #4757

Note, to test this in the UI requires a fix for #4833.  That fix, at the
time of crafting this commit message, can be seen at PR #4885.
jeremyf added a commit that referenced this issue May 4, 2021
Prior to this commit, we were not verifying that a user's input for a
release_date was in fact a parseable date.  Chrome and Firefox apply a
date picker "widget" that enforces entries for a `type="date"`.
However, Safari does not.

Closes #4757

Note, to test this in the UI requires a fix for #4833.  That fix, at the
time of crafting this commit message, can be seen at PR #4885.
no-reply pushed a commit that referenced this issue May 4, 2021
Prior to this commit, we were not verifying that a user's input for a
release_date was in fact a parseable date.  Chrome and Firefox apply a
date picker "widget" that enforces entries for a `type="date"`.
However, Safari does not.

Closes #4757

Note, to test this in the UI requires a fix for #4833.  That fix, at the
time of crafting this commit message, can be seen at PR #4885.
@jlhardes
Copy link
Contributor

jlhardes commented May 5, 2021

This is still giving an error on nurax-dev but it seems to be a different error than the original report: ActionController::UrlGenerationError in Hyrax::GenericWorks#create

See attached PDF file for full stack trace.
4833_Action Controller_ Exception caught.pdf

@jeremyf
Copy link
Contributor

jeremyf commented May 5, 2021

@jlhardes I believe the new error relates to #4889. Based on the exception, it looks like you're running SHA ff0986b which does not include 840a08f.

Below is the log of the main branch, at the top is the current "HEAD" of main. At the bottom is the commit prior to SHA ff0986b. Note that 840a08f occurs later in the log.

*   a8597884e — Merge pull request #4909 from samvera/gh-4900-edit-form-error-for-valkyrie Jeremy Friesen, (2021-05-04) (HEAD -> main, origin/main)
|\  
| * 01ecf2692 — Adding permision_populator to change set Jeremy Friesen, (2021-05-04)
|/  
* 9d51cb1fd — Adjusting lease/embargo_enforced helper logic Jeremy Friesen, (2021-05-04)
* 625a1a9eb — Adding rendering of lease related error messages Jeremy Friesen, (2021-05-04)
* 840a08ff8 — Fixing rendering logic for visibility component Jeremy Friesen, (2021-05-04)
* 7fe7786c6 — Ensuring non-parseable date bubbles up to UI Jeremy Friesen, (2021-05-04)
* ff0986b0f — Merge pull request #4908 from samvera/valkyrie-member-presenter Jeremy Friesen, (2021-05-04)
* 28f2086a6 — support full `MemberPresenterFactory` API in `PcdmMemberPresenterFactory` tamsin johnson, (2021-05-03)
* c08983cf5 — support member order in Valkyrie MemberPresenterFactory tamsin johnson, (2021-05-03)
* e78d3b24a — add a MemberPresenterFactory implementation that supports valkyrie tamsin johnson, (2021-05-03)

@jlhardes
Copy link
Contributor

jlhardes commented May 5, 2021

Tested again on nurax-dev now that it is fully updated with all changes for this fix and it works as expected (red alert message that date needs to be in the future and new work is not saved and edited work is not saved). This is done!

@jlhardes jlhardes closed this as completed May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants