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

feat: add a maximum order quantity #98

Draft
wants to merge 56 commits into
base: master
Choose a base branch
from
Draft

feat: add a maximum order quantity #98

wants to merge 56 commits into from

Conversation

yksflip
Copy link
Collaborator

@yksflip yksflip commented Aug 29, 2024 •
edited
Loading

ToDo:

  • csv/foodsoft file import
  • update api / swagger
  • check stock order bug:
    • +2 save
    • -1 --> 0
    • +2 impossible

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 22 commits July 26, 2024 10:34
@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 <[email protected]>
@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 <[email protected]>
@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
@lentschi
Fixes #86
bb044e6
@lentschi
Closes #77
cd2418c 
All newly added TODOs supplemented by GitHub issue links
@lentschi
Closes #10
3fca7de
@lentschi
Started with #23
99bc201 
- added missing keys used in sources to de/en
- fixes `import_search_results` key position in all locales
@coveralls
Copy link

coveralls commented Aug 29, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10653285104

Details

  • 13 of 15 (86.67%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+19.9%) to 68.151%
Changes Missing Coverage Covered Lines Changed/Added Lines %
app/models/group_order.rb 2 4 50.0%
Totals Coverage Status
Change from base Build 10229920942: 19.9%
Covered Lines: 4699
Relevant Lines: 6895
💛 - Coveralls

@yksflip yksflip force-pushed the feat/maximum-order-quantity branch from 8a88ad9 to 9cd0d22 Compare August 29, 2024 22:35
yksflip
yksflip commented Aug 29, 2024
View reviewed changes
app/models/order_article.rb
@@ -81,6 +81,8 @@ def update_results!
# 4 | 5 | 4 | 2
#
def calculate_units_to_order(quantity, tolerance = 0)
quantity = [price.maximum_order_quantity || quantity, quantity].min
return 0 if price.minimum_order_quantity && quantity < price.minimum_order_quantity
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if I misunderstand the expected behavior of minimum_order_quantity of this function, or if this case was just missing and just doesn't occur here as it is handled somewhere in client-side already?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right - at least at a first glance something seems broken here - I've created #99 to address the issue. (I'll take a closer look asap.)

@yksflip yksflip force-pushed the feat/maximum-order-quantity branch 2 times, most recently from 2665d81 to 1bafa9a Compare September 1, 2024 10:32
@yksflip yksflip force-pushed the feat/maximum-order-quantity branch from 1bafa9a to a9ccad9 Compare September 1, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentschi lentschi lentschi left review comments

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
@yksflip @coveralls @lentschi @twothreenine