Skip to content
This repository has been archived by the owner on Dec 13, 2022. It is now read-only.

Allow the results of one function to be the input for another function #96

Merged
merged 43 commits into from
Sep 16, 2020

Conversation

aguilerapy
Copy link
Contributor

@aguilerapy aguilerapy commented May 19, 2020

We have implemented #14 (comment) here. There are 3 different behaviors for the send button:

  1. Process and return the result on the same page in a new box, having both results on two different boxes. (chosen in this PR)
  2. Redirect us to the next page without processing yet, being able to modify options from the original function page or add more files. (more changes)
  3. Redirect us to the next page processing and returning the result. (without much difference with 1)

Right now, in the second step, it's only possible to go to 3 functions, due to the input/output file type:

  • Compile Package
  • Upgrade to 1.1
  • Convert to CSV/Excel

None of these functions have input options or parameters yet, except Compile Releases with "Include Versioned Releases" checkbox, so option 1 is the one implemented.

One possibility for option 1, is to show the parameters of the functions as we change the selectbox.

@yolile
Copy link
Contributor

yolile commented May 19, 2020

closes #94

@aguilerapy aguilerapy requested a review from romifz May 19, 2020 22:20
@romifz
Copy link
Contributor

romifz commented May 22, 2020

Hi @aguilerapy, sorry but I don't like too much the approach you took here.

The behaviour up to the selection of the next function is fine. But I don't think that staying on the same page is the best.

I know that most functions don't have input options yet but we are planning to add some like #82 and especially #26. If we stay on the same page after selecting the function to execute next, we'll have to duplicate efforts by putting input options in different places or rewrite this one.

When taking the results to another function you don't have to put them in the file upload box. Instead, for a first approach, you can show a simple list of the files generated by the previous function. In the future, we can think if it would be useful to allow the user to add more files or select the ones to use.

@aguilerapy
Copy link
Contributor Author

What do you think about the solutions proposed for this PR? @jpmckinney

@jpmckinney
Copy link
Member

I agree with @romifz's comment.

I assume that we can put the output files in the session. Right now, many views have a clear_files decorator, but maybe we need to change it to allow for exceptions when passing results to another function.

This was referenced Jun 3, 2020
@aguilerapy
Copy link
Contributor Author

aguilerapy commented Jun 18, 2020

Changes made to redirect to the page before processing the result.

The option to add more files to process them together with the sent result is pending (if necessary). Also, to send more than one result it's necessary to make changes (for example, after adding split-package in #99)

@aguilerapy aguilerapy mentioned this pull request Jun 18, 2020
3 tasks
# Conflicts:
#	default/static/js/uploader.js
#	default/templates/default/base-uploader.html
#	default/templates/default/to-json.html
#	default/templates/default/to-spreadsheet.html
#	locale/es/LC_MESSAGES/django.po
Copy link
Contributor

@romifz romifz left a comment

Choose a reason for hiding this comment

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

I tested the solution and I like that the upload box is filled in with the name of the result file. Did you consider the case of multiple output files, e.g. from the upgrade function? When testing I see that only one file is taken.

I think that the server-side solution is a little bit more complex than it needs to be with the request.session['clear'] variable. I also think it's no good to clear the files after a single call, what would happen there if the user reloads the page?

Instead of making an ajax call to save the result contents into request.session['files'] I would do the following:

  1. Always save a DataFile for the last generated result within a session variable.
  2. When a function receives the destination parameter in a GET request, the page displays the data of the last result as it is already doing now, but reading the data of the last result session variable. In the case of multiple result files, the session variable could be an array or the zip file info can be read here.
  3. In the POST request, use a parameter to indicate that the last saved result should be used as input. Here the easiest is to write a decorator that will unzip the file and insert the results into request.session['files'] before the method execution.

You should also avoid adding new buttons when they are not needed. The same 'Send' button should be used to execute the action. Having buttons that do very similar things and are in the same position, but appear alternately makes it difficult for another programmer to follow the code.

Also, why can't you send a file from compile to to-spreadsheet?

default/static/js/uploader.js Outdated Show resolved Hide resolved
default/static/js/uploader.js Outdated Show resolved Hide resolved
# Conflicts:
#	default/static/css/custom.css
#	default/templates/default/base.html
@aguilerapy aguilerapy requested a review from romifz August 8, 2020 01:45
default/decorators.py Outdated Show resolved Hide resolved
default/static/js/uploader.js Outdated Show resolved Hide resolved
default/templates/default/to-json.html Outdated Show resolved Hide resolved
locale/es/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/es/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
locale/es/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
# Conflicts:
#	default/static/css/custom.css
#	default/static/js/uploader.js
#	default/templates/default/base-uploader.html
#	default/templates/default/to-json.html
#	default/templates/default/to-spreadsheet.html
#	default/views.py
#	locale/es/LC_MESSAGES/django.po
@aguilerapy aguilerapy requested a review from romifz August 12, 2020 22:44
@aguilerapy
Copy link
Contributor Author

Since this is the last RP to merge based on #93, I'll add the help info panel.

@aguilerapy aguilerapy removed the request for review from romifz August 13, 2020 23:41
@aguilerapy aguilerapy requested a review from romifz August 19, 2020 17:51
default/util.py Outdated Show resolved Hide resolved
default/templates/default/base-tool.html Outdated Show resolved Hide resolved
# Conflicts:
#	default/templates/default/to-spreadsheet.html
#	default/views.py
#	locale/es/LC_MESSAGES/django.po
# Conflicts:
#	default/templates/default/package-releases.html
#	default/util.py
#	default/views.py
#	locale/es/LC_MESSAGES/django.po
Copy link
Contributor

@romifz romifz left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for working on the inclusion tag. I've left one comment for a single improvement I think you can do. Otherwise, looks good to go.

default/templates/default/base-tool.html Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants