FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

DMP Assistant Feature - allow phase-only download - Ready by pengyin-shan · Pull Request #3219 · DMPRoadmap/roadmap · GitHub

Repository navigation

DMP Assistant Feature - allow phase-only download - Ready - #3219

Merged
briri merged 14 commits into
developmentfrom
DMPAssistant-ISSUE70
Mar 23, 2023
Merged

briri merged 14 commits into
developmentfrom
DMPAssistant-ISSUE70

Conversation

Copy link
Copy Markdown
Contributor

Changes proposed in this PR:

  1. Disable Capybara::Webmock.start since Webmock complains about the use of this method (start is not there anymore I believe)

  2. New Feature from DMP Assistant: allow phase-only download for plans:

Now users can choose to download a single phase or all phases of the plan from the Download page (all formats except json).
A dropdown is provided to users on UI to select:

Then users could download the coversheet (if selected) and the phase they choose.

For CSV now, we allow user to download single phase only, no 'All Phases' avaible. We will add option to download all phases as a zip file later. If you want, you can disable this feature for csv for now by modifying the code (lin 25) in app/javascript/src/plans/download.js

briri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the changes look ok to me. And 👍🏻 for adding tests 😄

@martaribeiro, can you have a look when you get a chance? I think you have several multi-phase templates

Copy link
Copy Markdown
Member

Thank you @pengyin-shan. I'll try to look at it tomorrow. We have our dev server running with rails 6.

Copy link
Copy Markdown
Contributor Author

Thank you @briri and @martaribeiro ! Please let me know if you have any questions.

martaribeiro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I checked it locally. It works in rails 6 and in a 3 Phase plan :)

Great feature. Thank you

Copy link
Copy Markdown
Contributor Author

I checked it locally. It works in rails 6 and in a 3 Phase plan :)

Great feature. Thank you

Thank you @martaribeiro ! It also works on single-phase plans on your side right? (like the simple plans that don't have separate phase)

martaribeiro commented Oct 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

So when there is only one phase, the drop down doesn't display, which makes sense. But the PDF file does not include any questions or answers... @pengyin-shan I didn't check this before. I only tried the plan with multiple phases, and it did work when I selected all phases or a single phase. Not sure why it doesn't work for a single phase plan, as this was ok before. I should say I can see the plan details in the PDF, just not the questions or answers

Copy link
Copy Markdown
Contributor Author

So when there is only one phase, the drop down doesn't display, which makes sense. But the PDF file does not include any questions or answers... @pengyin-shan I didn't check this before. I only tried the plan with multiple phases, and it did work when I selected all phases or a single phase. Not sure why it doesn't work for a single phase plan, as this was ok before. I should say I can see the plan details in the PDF, just not the questions or answers

@martaribeiro sorry I thought the new pushes will be automatically notified to reviewers, but seems not...can you try again? I added one more commit

Copy link
Copy Markdown
Member

@pengyin-shan I'll try later tonight or tomorrow morning if it's ok.

Copy link
Copy Markdown
Contributor Author

@pengyin-shan I'll try later tonight or tomorrow morning if it's ok.

No problem! Please take your time and thank you a lot!!

Copy link
Copy Markdown
Member

@pengyin-shan I tried again and the same problem happens. When there is only one phase and I download a pdf, it's empty. No questions. I attached some screen grabs. Not sure what is happening

pengyin-shan changed the title DMP Assistant Feature - allow phase-only download PENDING_FOR_Rails6_TEST: DMP Assistant Feature - allow phase-only download Nov 7, 2022

briri commented Nov 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

I get the same result as @martaribeiro here. The issue lies with the following line in the plan_exports_controller.rb

@selected_phase = @plan.phases.order('phases.updated_at DESC')
                                             .detect { |p| p.visibility_allowed?(@plan) }

The visibility_allowed? function returns false unless 50% of the Plan's questions have been answered. I think it's safe to remove that .detect call.

To replicate, try generating a PDF for a plan (based on a template with a single phase) that does not have more than 50% of questions answered

Copy link
Copy Markdown
Contributor Author

I get the same result as @martaribeiro here. The issue lies with the following line in the plan_exports_controller.rb

@selected_phase = @plan.phases.order('phases.updated_at DESC')
                                             .detect { |p| p.visibility_allowed?(@plan) }

The visibility_allowed? function returns false unless 50% of the Plan's questions have been answered. I think it's safe to remove that .detect call.

To replicate, try generating a PDF for a plan (based on a template with a single phase) that does not have more than 50% of questions answered

Thank you @briri and @martaribeiro ! I see why I cannot duplicate the behavior you have. In DMP Assistant we set default_percentage_answered = 0 in _dmproadmap.rb instead of 50.

I'll remove .detect and push again

Copy link
Copy Markdown
Contributor Author

@briri sorry I want to confirm again before I made modifications since this seems to be a different feature change without concerning the 'All Phases' feature there is here or not.

We used to have all the 'uncompleted' questions & answers not downloadable. 'Uncompleted' or 'Completed' status is based on the default_percentage_answered setting.

By removing this line, we won't care about 'Uncompleted' or 'Completed' anymore. Any 'Uncompleted' questions and answers will be downloadable.

If this is what we need, let me know and I'll include this in the PR description.

briri commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

Hmm. That's a good question @pengyin-shan

I guess we still would want to hide uncompleted answers. Instead of making the change I suggested, can you update the logic in the view so that it outputs a message like _('No questions have been answered for this DMP.')

That way It will at least inform the user about why the PDF is empty. I'm open to other suggestions/options :)

Copy link
Copy Markdown
Contributor Author

Hmm. That's a good question @pengyin-shan

I guess we still would want to hide uncompleted answers. Instead of making the change I suggested, can you update the logic in the view so that it outputs a message like _('No questions have been answered for this DMP.')

That way It will at least inform the user about why the PDF is empty. I'm open to other suggestions/options :)

From the DMP Assistant perspective, we have seen users feel confused about default_percentage_answered to define answered/unanswered questions, so we just made it to 0.

In order to avoid such confusion for other roadmap users, how about _('Rest of the DMP hasn't been completed yet.')? user can still see 'completed' questions & answers.

pengyin-shan changed the title PENDING_FOR_Rails6_TEST: DMP Assistant Feature - allow phase-only download PENDING-need review for UI message. DMP Assistant Feature - allow phase-only download Nov 30, 2022

briri commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

@mariapraetzellis @dsisu can you recommend an appropriate message for this? The scenario is when someone exports their plan and has not answered any questions but has also selected not to display the question text. The resulting document is empty which is confusing and causes people to think there is an error.

pengyin-shan changed the title PENDING-need review for UI message. DMP Assistant Feature - allow phase-only download DMP Assistant Feature - allow phase-only download - Ready Feb 27, 2023

Copy link
Copy Markdown
Contributor Author

@martaribeiro : I verified this PR again by 1) config.x.plans.default_percentage_answered = 0 in_dmproadmap.rb then download 2) check 'unanswered questions' when downloading the plan, and the unanswered part will show. So your problem could link to this setting instead of the multi-phase download feature.

@briri : we discussed adding messages to let users know that since they haven't completed the plan, the rest of the content won't show. Unfortunately, I only have one day left so I don't have time to work on this feature. However, I created #3295 as a future reference.

Other than these, this PR is ready to go. It allows users to download both single-phase and in PDF, TEXT, and DOCX formats. Note that 1) CSV file can only download a single phase instead of all phases 2) JSON doesn't have this feature yet.

If you want to wait and discuss more of the single-phase download feature before merging this PR, please talk to Omar or Shiloh.

briri commented Mar 23, 2023

Copy link
Copy Markdown
Contributor

I think this is ok to merge but lets discuss in the next team meeting

briri commented Mar 23, 2023

Copy link
Copy Markdown
Contributor

going to merge this. I have tested it manually and It works as-is for single phase plans and I like the phase selector when the template has multiple phases.

briri merged commit 385e884 into development Mar 23, 2023
briri deleted the DMPAssistant-ISSUE70 branch March 23, 2023 21:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL