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

Issue202: Add contributor to the DMP downloaded files #206

Merged
merged 5 commits into from
May 24, 2022

Conversation

pengyin-shan
Copy link

Fixes #202 .

Changes proposed in this PR:

  • Added contributors to coverage of plans. Users will see both roles and contributor names if the role is filled ('Other' is replaced by 'Contributor' as the role name). A role can have many contributors and the same contributor can have more than one role:

Screen Shot 2022-05-19 at 17 25 54

  • For JSON export, it will show as part of JSON object with the whole contributor information:

"contributor":[{"name":"Wendy Shan Both","mbox":"[email protected]","role":["http://credit.niso.org/contributor-roles//data-curation","http://credit.niso.org/contributor-roles//investigation"],"affiliation":{"name":"University of British Columbia","abbreviation":"UBC","region":null}},{"name":"Wendy Shan","mbox":"[email protected]","role":["http://credit.niso.org/contributor-roles//investigation","other"],"affiliation":{"name":"University of British Columbia","abbreviation":"UBC","region":null}}]

  • Added plan title to csv file

@github-actions
Copy link

1 Warning
⚠️ There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior.

Generated by 🚫 Danger

@pengyin-shan pengyin-shan requested a review from lagoan May 20, 2022 15:59
@pengyin-shan pengyin-shan marked this pull request as ready for review May 20, 2022 16:02
@pengyin-shan
Copy link
Author

Meanwhile, I submitted an issue to DMPRoadmap: DMPRoadmap#3179 about the problem we have in the related rspec file.

@@ -46,6 +46,13 @@ def show
@selected_phase = @plan.phases.order("phases.updated_at DESC")
.detect { |p| p.visibility_allowed?(@plan) }
end

#ISSUE205: add contributor to cover page
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add explanation to the behaviour of the code instead of referencing the issue number of the repository. If we ever change repositories the issue number will no longer make sense.

Copy link
Author

Choose a reason for hiding this comment

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

We should also add tests to make sure all the people listed actually show up on the reports.

Yeah, I was also considering this. I submitted the issue DMPRoadmap#3179 to DMPRoadmap because our current way of testing is not tracking the downloaded file, instead, it is just tracking the original page source.

So far I haven't figured out the best way of asking RSpec to track the content of the 'downloaded file' (i.e. docx, csv, pdf..) because it is part of the user's computer instead of the rails program. I will continue to think about it. Please let me know if you know anything I can refer it.

Comment on lines 104 to 108
#ISSUE205: add contributor to cover page
hash[:data_curation] = Contributor.where(:plan_id => id).data_curation
hash[:investigation] = Contributor.where(:plan_id => id).investigation
hash[:pa] = Contributor.where(:plan_id => id).project_administration
hash[:other] = Contributor.where(:plan_id => id).other
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as comments above.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -7,6 +7,21 @@
<%# Allow raw html (==) for plan_attribution as it has <b> tags %>
<p><%== plan_attribution(@hash[:attribution]) %></p><br>

<%# ISSUE205: print out contributors if exit %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as comment about referencing repository issues.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -7,6 +7,21 @@
<%# Allow raw html (==) for plan_attribution as it has <b> tags %>
<p><%== plan_attribution(@hash[:attribution]) %></p><br>

<%# ISSUE205: print out contributors if exit %>
<%# Roles are ranked by PI -> DM -> PA -> Other (if any) %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good explanation of the behaviour of the view!

<%= @hash[:attribution].many? ? _("Creators: ") : _('Creator:') %> <%= @hash[:attribution].join(', ') %>
<%# ISSUE205: print out contributors if exit %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

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

Nice work all around!

I have just a few thoughts on the comments you added and changing some syntax to follow the rest of the syntax used in the application

@lagoan
Copy link
Collaborator

lagoan commented May 20, 2022

We should also add tests to make sure all the people listed actually show up on the reports.

Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

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

Excellent

@pengyin-shan pengyin-shan merged commit 926b69c into integration May 24, 2022
@pengyin-shan pengyin-shan deleted the issue202 branch May 24, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants