-
Notifications
You must be signed in to change notification settings - Fork 36
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
MMT-1019 MMT-1273 publish service drafts and published service previe… #223
Conversation
…w page Conflicts: lib/json_schema_form/umm_preview.rb
add_breadcrumb breadcrumb_name(@service, 'service'), service_path(params[:id]) | ||
end | ||
|
||
def edit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment out unused methods
@schema.fetch_references(@schema.parsed_json) | ||
end | ||
|
||
def set_form |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this method used
latest = service_data.first | ||
meta = latest.nil? ? {} : latest.fetch('meta', {}) | ||
|
||
if !@revision_id.nil? && meta['revision-id'].to_s != @revision_id.to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might introduce a bug we have for variables where after publishing a new revision, you still might see a banner saying you are viewing an old revision. MMT-1186. I think the solution might just be to add an else
statement to set the ivar to false
|
||
def initialize(schema_type:, preview_filename:, resource:) | ||
super(schema_type, preview_filename) | ||
# schema_type: published_resource_name (service/variable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reasoning for this to be the singular form, rather than plural_published_resource_name
? It seems that all the other UMM classes use the plural, and you're having to call .pluralize
most of the time schema_type
is used, including to call super
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is used 4 places, 2 of them need singular a 2 need plural, I liked the idea of using the simpler version in the param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it's a bit odd to have to change it when you're calling super
, but I guess it might be more a matter of opinion.
@@ -0,0 +1,77 @@ | |||
require 'rails_helper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this one of the tests that got 'lost'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep,
service_drafts/preview/*/service_information_preview_spec.rb
and
service_drafts/preview/*/service_keywords_preview_spec.rb
…w page
Conflicts:
lib/json_schema_form/umm_preview.rb