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

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

Merged
merged 14 commits into from
Mar 23, 2023

Conversation

pengyin-shan
Copy link
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:
Screen Shot 2022-10-03 at 1 29 57 PM
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

Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

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

@martaribeiro
Copy link
Contributor

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

@pengyin-shan
Copy link
Contributor Author

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

Copy link
Contributor

@martaribeiro martaribeiro left a comment

Choose a reason for hiding this comment

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

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

Great feature. Thank you

@pengyin-shan
Copy link
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
Copy link
Contributor

martaribeiro commented Oct 8, 2022

Screenshot 2022-10-08 at 11 18 15

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

@pengyin-shan
Copy link
Contributor Author

Screenshot 2022-10-08 at 11 18 15

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

@martaribeiro
Copy link
Contributor

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

@pengyin-shan
Copy link
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!!

@martaribeiro
Copy link
Contributor

@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

Screenshot 2022-10-12 at 13 44 36

Screenshot 2022-10-12 at 13 48 22

Screenshot 2022-10-12 at 13 15 11

@pengyin-shan 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
Copy link
Contributor

briri commented Nov 29, 2022

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

@pengyin-shan
Copy link
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

@pengyin-shan
Copy link
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
Copy link
Contributor

briri commented Nov 29, 2022

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 :)

@pengyin-shan
Copy link
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 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
Copy link
Contributor

briri commented Jan 23, 2023

@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 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
@pengyin-shan
Copy link
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
Copy link
Contributor

briri commented Mar 23, 2023

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

@briri
Copy link
Contributor

briri commented Mar 23, 2023

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 briri merged commit 385e884 into development Mar 23, 2023
@briri briri deleted the DMPAssistant-ISSUE70 branch March 23, 2023 21:01
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.

3 participants