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

Accept asset/accessory improvements and fixes #11172

Merged
merged 21 commits into from
May 23, 2022

Conversation

snipe
Copy link
Owner

@snipe snipe commented May 20, 2022

We're seeing a handful of problems and inconsistencies with asset acceptance, so this is a whack at refactoring some of that.

Problem 1:

The storage/eula-pdfs directory did not have a .gitignore in it, which means it wasn't checked into the repo, so users trying to store the PDF would end up with errors, since the file couldn't be written to a non-existent directory. (We were also not checking to see the directory actually exists before trying to write to it.)

Problem 2:

We had added an extra column to the action_logs table called stored_eula_file, but that ended up being difficult to normalize in the API and the UI - and we already have a filename column on that table, so it makes more sense to just use that. (I'm still not entirely sure why we're duplicating the all of the data in the action_logs table in the checkout_acceptances table, but that's for another time.)

Normalizing this format means we go from something like this:

Screen Shot 2022-05-16 at 11 10 55 PM

to something like this:

Screen Shot 2022-05-17 at 7 17 03 AM

where the "uploads" and "acceptance pdfs" both show up in a unified way in the history tab.

Problem 3:

The stored_eula, the actual EULA text at the time of the acceptance, is being stored but not displayed anywhere. It will likely be long, and with the PDF support, I don't see any reason to try to cram it into the UI un the history tab.

I still need to do a little refactoring in the acceptance controller and clean that up a bit, but I think this isn't a bad start.

I also cleaned up the PDF layout a bit, so it looks like this now:

Screen Shot 2022-05-17 at 7 23 39 AM

Still a few more tweaks to go, but I think it looks a little cleaner.

This will eventually hopefully fix #11116.

snipe added 21 commits May 19, 2022 17:55
Signed-off-by: snipe <snipe@snipe.net>
It’s extraneous, since we already have a file field in the action_logs, and we already store the stored_eula_file in checkout_acceptances.

Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
@snipe snipe requested review from inietov and uberbrady May 20, 2022 01:01
@snipe snipe merged commit 8333089 into develop May 23, 2022
@snipe snipe deleted the rebased_added_gitkeep_to_to_eula_pdfs branch May 23, 2022 19:59
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.

None yet

1 participant