search

ucsdlib / damspas-rd

A Digital Collections application based on Hyrax
MIT License
3 stars 2 forks source link

Fixed #84, #85, #92, #59 - Implemented the new rights metadata model with local customized visibilities. #108

Closed lsitu closed 7 years ago

lsitu commented 7 years ago

Fixes #84, #85, #92, #59

Implemented the new rights metadata model with support for the edit form, Batch Import and UI display.

Changes proposed in this pull request:

lsitu commented 7 years ago

@ucsdlib/developers Please review, comments ... Thanks.

hweng commented 7 years ago

Looks :+1: to me, and I agree with @mcritchlow comments above.

lsitu commented 7 years ago

@ucsdlib/developers I've updated the PR to use the new pattern to fetch field values from solr_doc as @mcritchlow suggested. Please review, comments ... Thanks.

BTW, It seems like that the develop branch of the current horton codebase is broken with error "uninitialized constant ActionView::CompiledTemplates::Flipflip", an obviousFlipflop typo. And I can't rebase it. I think we must have been upgraded to a wrong hyrax version recently! Does anyone see that error?

Here are the steps to replicate it after checking out the latest develop branch from https://github.com/ucsdlib/horton: From the dashboard, click on menuWorks on the middle of the left panel (or http://localhost:3000/dashboard/my/works?locale=en).

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 92.26% when pulling f3025d23ab8bb09792ad5c65b4ece580e28a2f3e on feature/visibility_rights into 776282e40c00812c134428885e399ba4855ea117 on develop.

hweng commented 7 years ago

@lsitu The version didn't change which is still '2.0.0.alpha'. It was upgraded to the latest commit at that point: projecthydra-labs/hyrax@562b8ff, see https://github.com/ucsdlib/horton/pull/109, as the ticket required: https://github.com/ucsdlib/horton/issues/93,
And all our local tests passed.

As you know the hyrax is keeping on developing, it is not surprised if it is conflict with our local development and we need to stay up to date with Hyrax development. You may upgrade to the latest commit from hyrax master branch to see if it works fine to you.

lsitu commented 7 years ago

@hweng It was fixed 14 days ago, just shortly after the hyrax commit that are used for horton upgrade: https://github.com/projecthydra-labs/hyrax/commit/6688333c9343fd7d2e19594661522a826d871951

hweng commented 7 years ago

@lsitu Sounds good. Just upgrade to the latest commit. We might need to keep on upgrading it in each sprint.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-56.01%) to 35.835% when pulling fd079fac3bcea9cc42989b026cd6df38fba3a061 on feature/visibility_rights into 2583c57003ee97562e85d5dab4299cb849bfcf5d on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 92.492% when pulling 1914af48422ecadfcc6570825d4fa4b13efe86a8 on feature/visibility_rights into 2583c57003ee97562e85d5dab4299cb849bfcf5d on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.3%) to 92.1% when pulling c7c2d8302dd9d6d96f6aee675e441c2ba365ee3a on feature/visibility_rights into 2583c57003ee97562e85d5dab4299cb849bfcf5d on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 92.019% when pulling 46acc6cf2d23b005f4f39d68424e31c8d9187709 on feature/visibility_rights into 2583c57003ee97562e85d5dab4299cb849bfcf5d on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 91.96% when pulling 1071983f36b206356a59bbae0c4d85102d4141d0 on feature/visibility_rights into 2583c57003ee97562e85d5dab4299cb849bfcf5d on develop.

lsitu commented 7 years ago

@mcritchlow I tried to feed Rubocop to make it happy but still got it failed, though I've made lots of unnecessary changes for a shorter variable name, using if block instead of appending the modifier at the end, adding comments like "# frozen_string_literal: true" to all class/module, disabled line length check and method length check in some cases, manipulating for indentation, using the %i pattern as suggested for array etc. But I still can't get it passed for the following cases:

  1. "Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true": when the comment is added on the top of the class definition.
  2. "Layout/EmptyLineBetweenDefs: Use empty lines between method definitions.": with method authorize_download! from hyrax that is overridden.
  3. "Style/SymbolArray: Use %i or %I for an array of symbols.", while complaining about "Style/PercentLiteralDelimiters: %i-literals should be delimited by [ and ]." when something like %i{one two three} applied.

During feeding Rubocop to fix the errors, there are some inconsistency message like "Layout/TrailingBlankLines: Final newline missing.", "Layout/IndentationConsistency: Inconsistent indentation detected.", "Performance/StringReplacement: Use tr instead of gsub." etc.

It seems like we should run into the same situation that hyrax do. I think we may need to consider the following changes:

  1. Reasonable longer line length, maybe 100 or 120?
  2. Allow more lines for methods, maybe 14 to make it inline with hyrax?
  3. Use blanket pattern for arrays.
  4. Disable FrozenStringLiteralComment to avoid "Missing magic comment # frozen_string_literal: true" error.
mcritchlow commented 7 years ago

@lsitu, this should probably be a team decision since everyone is impacted, so @VivianChu and @hweng please give a 👍 or 👎 to each of Longshou's proposed changes to the rubocop config above.

I think it is a very good argument to align with the Hyrax rubocop conventions since we periodically have to override parts of that codebase. My thoughts on each:

  1. 👍 I think up to 120 is fine
  2. 👍 seems like a reasonable compromise
  3. 👍 I don't have strong preferences on this.
  4. I could go either way on this. I hadn't heard of this one, to be honest. But it looks like it's trying to give a warning/heads up about the Ruby 3 migration path. Does adding the comment line # frozen_string_literal: true as noted in the cop description make this cop happy?
lsitu commented 7 years ago

@mcritchlow For #4, the cop just won't be happy with comment # frozen_string_literal: true added on the top of the module definition for modules like FileSetIndexer and IndexesAttributes even. But I can give it another try to add it to the body of the module definition to see how it goes.

mcritchlow commented 7 years ago

@lsitu if it didn't work, I'd say disable as you proposed. Not worth the hassle then, imo

hweng commented 7 years ago

It is fine for me :+1:. Overall we have to agree that the rubocop is a great tool which points out some bad style in your code to fix.

VivianChu commented 7 years ago

👍

lsitu commented 7 years ago

@ucsdlib/developers Per our discussions above, I've added PR https://github.com/ucsdlib/horton/pull/114 for rubocop configuration. Please review and comments. Thanks.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.4%) to 92.259% when pulling c6601e9d9a7bed9307c39f52e218c35279255a1e on feature/visibility_rights into 3c0442831f1002cb0c2fd8357127f4cce8bb83d7 on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.4%) to 92.263% when pulling e503d5655ce5011752aa9c4b1a7905b6f26fa7ec on feature/visibility_rights into 3c0442831f1002cb0c2fd8357127f4cce8bb83d7 on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.4%) to 92.267% when pulling e9be13e571058c5c1b964e7dd59e858ec4484373 on feature/visibility_rights into d712e83c25bd8beb29d87a9baec3a3480217fb28 on develop.

lsitu commented 7 years ago

@ucsdlib/developers The style check is passed now. Please review and comments ... Thanks.

VivianChu commented 7 years ago

👍