Mount Oaisys inside Jupiter [ch109] #1361

ConnorSheremeta · 2019-11-27T20:06:30Z

Added scopes, inflection, config for oaisys, and mounted the engine to the route.

ConnorSheremeta · 2019-11-27T21:54:02Z

CI should pass once ualbertalib/oaisys#17 goes through and is referenced in this Gemfile

mbarnett · 2019-11-27T23:29:26Z

app/models/thesis.rb

@@ -9,6 +9,7 @@ class Thesis < JupiterCore::Doiable
  has_many_attached :files, dependent: false

  scope :public_items, -> { where(visibility: JupiterCore::VISIBILITY_PUBLIC) }
+  scope :belongs_to_set, ->(set) { where('member_of_paths::text LIKE ?', "%#{set}%") }


Just so we're consistent in our terminology (which is only going to get worse in phase 2 as the Community & Collection nesting gets joined by other nested relationships), let's use 'path' instead of set.

Also maybe throw a TODO on this so that once we've upgraded to Postgres 12 we come back and upgrade this query to use proper JSON operators instead of this terrible trick I came up with of casting the field to text.

…oaisys ticket

mbarnett · 2019-11-28T21:55:40Z

app/models/item.rb

+  # with an upgraded version of Postgresql this could be done more cleanly and performanetly
+  scope :belongs_to_path, ->(path) { where('member_of_paths::text LIKE ?', "%#{path}%") }
+  scope :record_created_from, ->(date) { where('record_created_at >= ?', date) }
+  scope :record_created_until, ->(date) { where('record_created_at <= ?', date) }


Really tiny nit, but I think these names are slightly confusing? Or at least I feel like I have to stop and think about what from and until mean, and that I'd misinterpret the behaviour and maybe be surprised by the "up to and including" nature of the queries.

Maybe something like created_on_or_before(date) and created_on_or_after(date)? Just so it reads fluently and there's no confusion as to what something like Item.created_on_or_before(date) returns when they start being used in non-OAI contexts (where there's going to be less awareness of what from and until mean in the OAI API context)

mbarnett

…ameters config

ConnorSheremeta · 2019-12-04T22:05:28Z

Some comments on my second to last commit... from the other oaisys PR:

Found some exceptions thrown regarding unpermitted parameters when running tests.

When hitting /search with the header (Content-Type: application/json) or when running get search_url, as: :json (which the test does.) the exception is thrown (ActionController::UnpermittedParameters: found unpermitted parameter: :search ). The search param is present but it is an empty hash which is the reason the exception is thrown ("search"=>{}). One way to fix is is just supplying empty an empty string as a search parameter get search_url, as: :json, params: { search: '' } I am unsure as to why this happens when requesting a json response but I don't think that is done other than in tests?

I also found tests failing regarding draft theses... found unpermitted parameters: :date_created(3i), :date_created(2i) happens when trying to move to step two of the wizard for a thesis. Those parameters are created on line 56 and 58 of draft_actions.rb and are not in the permitted_attributes array in draft_thesis_policy.rb. I've added a check for @draft.is_a?(DraftItem) before setting the month and day automatically because date_created doesn't pertain to theses.

I think these are appropriate fixes, we could still revert to checking the parameters manually if you think there could be too many complications, but I think they're all cleared up.

ConnorSheremeta · 2019-12-09T18:40:29Z

Found an issue from doing config.action_controller.action_on_unpermitted_parameters = :raise... if the page is changed an exception is thrown as :page is not a permitted param on line 3 of search helper. I've made it permitted and added an .except(:page) which should not change anything as the default behaviour was to just filter the unpermitted parameters anyways

murny · 2020-01-02T22:36:52Z

app/models/thesis.rb

+  # testing whether a value is in a json array, which newer version of Postgresql have.
+  #
+  # with an upgraded version of Postgresql this could be done more cleanly and performanetly
+  scope :belongs_to_path, ->(path) { where('member_of_paths::text LIKE ?', "%#{path}%") }


Very minor, just curious if we should worry about case sensitivity here? e.g use ILIKE instead of LIKE?

Maybe doesn't matter here?

Good question...

You could make the argument that case-insensitivity would be cleaner/work if someone manually typed a URL with all-caps. This is all just a terrible hack to work around the fact that the version of Postgres in Prod is currently 9.2 and 9.2 doesn't really support proper JSON queries that would let us ask whether that string is in the member_of_paths array. We're now building a new Production environment with Postgresql 12 that supports the proper query syntax, so this should just be switched to use the proper query instead.

I'll create a ticket/ quick PR to upgrade this query.

murny · 2020-01-02T22:50:33Z

config/application.rb

@@ -39,5 +39,8 @@ class Application < Rails::Application

    config.redis_key_prefix = "jupiter.#{Rails.env}."

+    # Set an action on unpermitted parameters to raise an exception, used to validate parameters in Oaisys.
+    config.action_controller.action_on_unpermitted_parameters = :raise


Curious the impact of this would be in production? Makes sense in Dev/Test over :log but with production could be troublesome perhaps and could result in a lot of crashes that would be difficult to troubleshoot?

For example say someone links a page on ERA on a 3rd party site, let's say Slack. Slack may add additional params to the link, e.g era.ualberta.library.ca/search?q=thesis&utm_source=slack for tracking. This would be an unpermitted param? Which means anyone clicking that link will get served a 500 error

Couldn't find much online to confirm this, except for this stackoverflow article:
https://stackoverflow.com/questions/57461284/action-on-unpermitted-parameters-on-production-environment
which highlights these same concerns.

Would you be able to verify this?

And if that is the case, maybe we just want to enable this in dev/test only

Good points, it has been decided that were going to set it to raise for dev/test and false in prod. Going to go back to manually checking oai parameters.

murny

Got some small concerns on setting config.action_controller.action_on_unpermitted_parameters = :raise in production. Can you double check this?

ConnorSheremeta · 2020-01-07T17:15:11Z

Found an issue from doing config.action_controller.action_on_unpermitted_parameters = :raise... if the page is changed an exception is thrown as :page is not a permitted param on line 3 of search helper. I've made it permitted and added an .except(:page) which should not change anything as the default behaviour was to just filter the unpermitted parameters anyways

Some more context on the page parameter not being permitted in the place above. The page parameter is being permitted and used on line 64 of search_controller. Because it is permitted there doesn't mean it is permitted elsewhere as .permit returns a new ActionController::Parameters instance

…isys-inside-jupiter

…cent oaisys commit

ConnorSheremeta · 2020-01-10T19:51:39Z

Note: this PR should probably be merged once the gemfile references an oaisys repo with all list identifiers commits (2 PRs are still open in oaisys)

ConnorSheremeta · 2020-01-10T22:48:44Z

Now referencing final Oaisys list identifiers commit in the gemfile, ready to be merged (once approved).

mbarnett

Looks good to me

Mount Oaisys inside Jupiter [ch109]

4b33a71

ConnorSheremeta requested a review from mbarnett November 27, 2019 20:06

ConnorSheremeta added 2 commits November 27, 2019 13:11

added changelog entry

1826af8

fix rubocop violations

b2dc66e

mbarnett suggested changes Nov 27, 2019

View reviewed changes

addressed comments from code review and suck in more scopes for next …

6ed5c38

…oaisys ticket

mbarnett suggested changes Nov 28, 2019

View reviewed changes

addressed comments from second review

f1d47b1

ConnorSheremeta requested a review from mbarnett November 28, 2019 23:20

mbarnett previously approved these changes Nov 28, 2019

View reviewed changes

Add different behaviour for action_on_unpermitted_parameters

02cb4e0

ConnorSheremeta dismissed mbarnett’s stale review via 02cb4e0 December 2, 2019 23:02

ConnorSheremeta added 2 commits December 3, 2019 16:24

doing action controller config properly now

de6fd04

fix exceptions being thrown due to changing action_on_unpermitted_par…

b4f2375

…ameters config

ConnorSheremeta mentioned this pull request Dec 4, 2019

Verb ListIdentifiers [ch101] ualbertalib/oaisys#17

Merged

oops, forgot my binding.pry

5686ecf

.except page because it was getting ignored before anyways

89acd46

rubocop offense

e85b97a

ConnorSheremeta force-pushed the sheremet/ch109/mount-oaisys-inside-jupiter branch from a75d9be to e85b97a Compare December 13, 2019 17:22

murny reviewed Jan 2, 2020

View reviewed changes

murny suggested changes Jan 2, 2020

View reviewed changes

ConnorSheremeta added 3 commits January 10, 2020 11:11

Merge branch 'integration_postmigration' into sheremet/ch109/mount-oa…

543c23a

…isys-inside-jupiter

set action_on_unpermitted_parameters properly and now getting most re…

31e23c4

…cent oaisys commit

found another unpermitted param which was causing system tests to fail

1a33483

ConnorSheremeta requested a review from murny January 10, 2020 19:01

This was referenced Jan 10, 2020

Create tests for list identifiers response [ch111] #1377

Merged

Modify list identifiers tests to test resumption [ch112] #1381

Merged

now referencing final list identifiers commit

9b9ce3b

mbarnett approved these changes Jan 10, 2020

View reviewed changes

murny approved these changes Jan 10, 2020

View reviewed changes

ConnorSheremeta merged commit c474009 into ualbertalib:integration_postmigration Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mount Oaisys inside Jupiter [ch109] #1361

Mount Oaisys inside Jupiter [ch109] #1361

ConnorSheremeta commented Nov 27, 2019

ConnorSheremeta commented Nov 27, 2019

mbarnett Nov 27, 2019

mbarnett Nov 28, 2019

mbarnett left a comment

ConnorSheremeta commented Dec 4, 2019

ConnorSheremeta commented Dec 9, 2019

murny Jan 2, 2020

mbarnett Jan 6, 2020

murny Jan 2, 2020 •

edited

Loading

ConnorSheremeta Jan 6, 2020 •

edited

Loading

murny left a comment

ConnorSheremeta commented Jan 7, 2020

ConnorSheremeta commented Jan 10, 2020

ConnorSheremeta commented Jan 10, 2020

mbarnett left a comment

Mount Oaisys inside Jupiter [ch109] #1361

Mount Oaisys inside Jupiter [ch109] #1361

Conversation

ConnorSheremeta commented Nov 27, 2019

ConnorSheremeta commented Nov 27, 2019

mbarnett Nov 27, 2019

Choose a reason for hiding this comment

mbarnett Nov 28, 2019

Choose a reason for hiding this comment

mbarnett left a comment

Choose a reason for hiding this comment

ConnorSheremeta commented Dec 4, 2019

ConnorSheremeta commented Dec 9, 2019

murny Jan 2, 2020

Choose a reason for hiding this comment

mbarnett Jan 6, 2020

Choose a reason for hiding this comment

murny Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

ConnorSheremeta Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

murny left a comment

Choose a reason for hiding this comment

ConnorSheremeta commented Jan 7, 2020

ConnorSheremeta commented Jan 10, 2020

ConnorSheremeta commented Jan 10, 2020

mbarnett left a comment

Choose a reason for hiding this comment

murny Jan 2, 2020 •

edited

Loading

ConnorSheremeta Jan 6, 2020 •

edited

Loading