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.

Sign up for GitHub

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

Jump to bottom

Adapt article & group sorted order PDF (solves #49) #87

Open
wants to merge 52 commits into
base: master
Choose a base branch
from
Open

Adapt article & group sorted order PDF (solves #49) #87

wants to merge 52 commits into from

Conversation

twothreenine
Copy link
Contributor

Since there weren't any "ready for PR" issues left which I could work on (realistically), I worked on #49.

I also adapted the PDF sorted by groups to show the ordered amount in group order unit and the received amount in billing unit.
Matrix PDF remains to be adapted.

If you think this isn't ready to be merged, we can also keep it as a draft and later merge it on upstream (in order to focus on the PR).

lentschi and others added 30 commits July 26, 2024 10:34
@lentschi
Article units squash
ecf7c17
@lentschi
Fix typos and removed dead code
cfbe5b7
@lentschi
Remove obsolete testing shared db along with its tasks
2a42360
@lentschi
Added no-upstream for byebug
539613b
@lentschi
On #47 Adapt unit ratio export/import to be interpreted as a conversi…
ba11e4b 
…on chain instead of a conversion table
@lentschi
On #21: stock orders: prohibit 'ordering' more than what's on stock
5bcea4a
@lentschi
Started with #8
b39cb77 
- made article model spec work again
- removed some more dead shared db code
@lentschi
Fixes #53
51c2703 
Explanation of why this only occurs now: The previous rails version ignored invalid ids
for `accepts_nested_attributes_for` parameters, the new one doesn't.
-> We need to avoid sending them.
@yksflip @lentschi
fix: round article quantity in order article pdf
37d1b27
@lentschi
Don't show articles migrate button for newly created suppliers
d831092
@lentschi
On #8: All existing specs pass now
acc982d 
This also fixes several actual bugs (partially caused by the rabase) detected by the specs
@lentschi
Rubocop fixes
e5a2398
@lentschi
First proposal for #35
74ac2f3
@lentschi
On #15: Open with inset button instead of on every focus
cb38646
@lentschi
Fixes #56
8b6d01a
@lentschi
On #35: Adapted list of default units
90c4fcb
@lentschi
Fixes #42
1c9612c
@lentschi
Fixes #42 again
4106ee8 
Changed decimals to 3 as requested
@lentschi
On #8: Implemented article version model unit tests
0abb392
@lentschi
On #8: Adapted article version unit tests
eb33fe8
@lentschi
On #35 Added missing metric default unit: GRM
8ed4bb9
@lentschi
Fixes #56 again
be44773
@lentschi
Fixes #59
6eb79bf
@lentschi
Fixes #58
a51304b
@lentschi
Fixes #60
3939556
@lentschi
Fixes #65
3941539
@lentschi
Fixes #62
6c77080
@lentschi
On #8: Fixes failing tests on github
476e322
@lentschi
On #8: Article units integration tests
3ce6390
@lentschi
On #8: Started with unit tests for supplier article sharing
c67a465
lentschi and others added 21 commits July 26, 2024 10:34
@lentschi
Fixes #68
de4792a
@lentschi
Fixes #67
b61b847
@lentschi
Fixes #69
2e19cd3
@lentschi
On #8: Done with sync specs
2c02208
@lentschi
On #15: Fixes conversion popover min-width
67041a1
@lentschi
Fixes #61
2ae2c6d
@lentschi
Fixes #73
0754f15
@lentschi
Better alias for #71
222ada6
@lentschi
On #64: Add missing unit labels
97a3f48
@lentschi @twothreenine
On #42: Fax pdf/csv: Decimals dependant on supplier_order_unit's si c…
602cff1 
…onvertability (#70)

* On #42: Fax pdf/csv: Decimals dependant on supplier_order_unit's si convertability

* Solve #42: Improve fax PDF, CSV, text

- outsource format_units_to_order to OrderHelper
- fax text: include unit, adjust column width
- fax PDF & text: only include order number if any present

* On #42:

- Adapted order_txt to generalized creating the text table and added
  spec
- Code style fixes for order_fax

* On #42 Fixes error dad0bb9#r143648091

---------

Co-authored-by: twothreenine <leonard_ostler@yahoo.de>
@yksflip @lentschi
feat: update seed files (#76)
366065f 
Closes #16 

Merged the hackathon and small seeds and updated the articles to handle the new unit system.
Dropped the .nl and .de seed files, because I think it's unnecessary work to further maintain them.
@lentschi @yksflip
Fixes #66 (#74)
8f87d7e 
* Fixes #66

* Fixes broken integration tests

* chore: refactor extract articles_helper article_version test setup function

* chore: make helper functions private

* On #66 Fixes https://github.com/foodcoopsat/foodsoft_hackathon/pull/74/files/b078cfaa87d334dba8eaafff5b2be152f293c448#r1670426999

---------

Co-authored-by: Philipp Rothmann <philipprothmann@posteo.de>
@lentschi
Workaround for #78
61a2b1a
@lentschi
Moved fork's migrations after rebase on upstream/master
681095b
@lentschi
Fixes #85
dec2762
@lentschi
Allow price calculation for new records
9191f42
@lentschi
Fixes #83
3871c37
@lentschi
Another workaround for #78
756fda0
@lentschi
Fixes #81
b3bc712
@twothreenine
Article CSV: fix #47 and #46 (#84)
3be31d5 
- rearrange CSV columns as I suggested in #47
- add locales for column headings according to my suggestions in #50 (to do: adjust terms across menus -- post-merge?)
- update documentation of CSV layout (#46)

TO DO:
any adaptations for rearranged tables necessary which I've overlooked? (for example sync feature)
@lentschi
Fix unit tests (broken since 3871c37)
3297e5e
@twothreenine twothreenine requested a review from lentschi July 29, 2024 17:23
@coveralls
Copy link

coveralls commented Jul 29, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10178232479

Details

  • 10 of 36 (27.78%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 67.893%
Changes Missing Coverage Covered Lines Changed/Added Lines %
app/documents/order_by_articles.rb 0 9 0.0%
app/lib/order_pdf.rb 6 23 26.09%
Totals Coverage Status
Change from base Build 10113367318: -0.2%
Covered Lines: 4688
Relevant Lines: 6905
💛 - Coveralls

@twothreenine twothreenine force-pushed the feature/adapt-article+group-order-pdf branch from 9594c1c to a8d87d7 Compare July 31, 2024 10:07
@twothreenine
Copy link
Contributor Author

I don't think this would make the fork "bigger" as it only affects files already affected by the fork.

@lentschi
Copy link
Contributor

lentschi commented Aug 3, 2024

I don't think this would make the fork "bigger" as it only affects files already affected by the fork.

Yeah, but it affects more considerably more lines in those files. Also none of this is unit tested and thus neither is you new "total" logic (which btw. has nothing to do with the units fork).

As for the change itself: It looks good to me in general. I'm just not sure, if everyone would agree with the visible changes in the pdf.

Cons I see (admittedly minor):

  1. More text to read - a lot of it repetitive (E.g. if many groups order beer, you will have just as many occurrences of <amount> Bottle (0.5l), which is a bit annoying)
  2. Ordered and Received columns are no longer easily comparable. (E.g. for the hackathon "Smoked tofu": 2 Piece (160 g) |320.000 g. For many numbers, this might be arduous.)

Long story short: I'm not saying that in total this PR wouldn't be an improvement, I just think it should rather be done and discussed upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentschi lentschi Awaiting requested review from lentschi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@twothreenine @coveralls @lentschi @yksflip