Manage excluded_products in ranges dashboard #4073
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4073 +/- ##
==========================================
- Coverage 87.42% 87.39% -0.04%
==========================================
Files 291 291
Lines 15858 15932 +74
==========================================
+ Hits 13864 13923 +59
- Misses 1994 2009 +15
|
|
Added missing tests. where does the "1 file has unexpected coverage changes not visible in diff " error come from? |
There was a problem hiding this comment.
Hi @gwaidacher , thank you very much for your PR. It's quite a big one and we really appreciate it that you're taking the time for it!
I do have some review comments though, if you have any questions or comments to mine, just let me know! :)
|
PR updated, please have a look |
There was a problem hiding this comment.
Hi @gwaidacher , I still have some feedback to keep it a bit more clean. Thanks again for your time and effort.
| self.handle_file_products(request, range, form) | ||
| return redirect( | ||
| reverse('dashboard:range-products', kwargs={'pk': range.pk}) | ||
| + '?upload_type=excluded' |
There was a problem hiding this comment.
I don't think this is really necessary. Once the upload was successful, we can just redirect them as is, maybe with a success message stating it was successful. The GET parameter is not really needed in my opinion.
There was a problem hiding this comment.
It's used to redirect to the page having the second tab with excluded products active.
| ) | ||
| return redirect( | ||
| reverse('dashboard:range-products', kwargs={'pk': range.pk}) | ||
| + '?upload_type=excluded' |
|
|
||
| if form.cleaned_data["upload_type"] == \ | ||
| RangeProductFileUpload.EXCLUDED_PRODUCTS_TYPE: | ||
| action = _('excluded from this range') |
There was a problem hiding this comment.
You forgot to actually add the product to the excluded products here
product_range.excluded_products.add(product)
There was a problem hiding this comment.
please add a test for this
There was a problem hiding this comment.
There is already a test for this in test_range: "test_dupe_excluded_skus_warning"
"'The products with SKUs or UPCs matching 456 have already been excluded from this range'"
| <div class="float-right"> | ||
| <input type="hidden" name="action" value="remove_excluded_products" /> | ||
| <button type="submit" class="btn btn-secondary" data-loading-text="{% trans 'Removing...' %}"> | ||
| {% trans "Remove excluded products" %} |
There was a problem hiding this comment.
Please change this to:
Remove selected excluded products
|
In general this is missing some tests, great that the existing tests still pass but please add some specific tests for excluding the products. |
viggo-devries
added
Needs more work
Needs tests
Hi! There are already 2 tests in test_range.RangeProductViewTest: test_upload_excluded_file_with_skus for the upload-file form and test_dupe_excluded_skus_warning - if this is insufficient, please specify which function should be tested. |
Yes I saw but the fact that your tests missed this entire functionality: f97053a is not good. I would like to see tests for every action that can be performed and also the result of that action. |
File Upload for excluded_products
added url to show tab with excluded_products
process excluded_products form
Form for excluded_products
Added nav-tabs for excluded_products
fix missing komma
fix parenthesis
fix continuation line
fix import redirect
fix import form
fix continuation line
added migration file
query first form in view
test first form for included_products
alter field id
changed id to AutoField
test RangeProductExcludedFileUpload
import corrected
continuation line
fix flake8
fix import order
added RangeProductExcludedFileUpload registry
author gwaidacher <gw@acat.cc> 1678894171 +0100
committer Joey Jurjens <joey@highbiza.nl> 1687521813 +0200
added Meta on AbstractRangeProductExcludedFileUpload
test 2nd form
restructured forms
fixed contination line
changed offer migrations
fix removed obsolete code
fix test_upload_excluded_file_with_skus
fix newline
set form.included = True
fix return form
add included to handle_file_products
updated error message
fix error msg in test
fix error msg
fix success msg
changed forms key
modified test
modified test url
test 3rd form
test form 2
added test for dupe skus
query form 3
better form 3?
should be form 2
add products
check form 2
context['form_excluded']
added action to msg
fixed message
fixed success msg
added test for removing excluded product
add test for removing included product
fixed url
fix submit form
improved clean_query
fixed german translation
test unexpected coverage changes
Restructured forms
fixed form test
updated translation strings
fixed format mapping
format mapping
format mapping
format mapping
updated test
fixed msg
++ "this"
no mapping
fix ngettext msg
undo changes
undo changes
pass excluded=True as kwarg
removed excluded from init args
update test_range.py
refactored forms
Update test_range.py
default for upload_type
added comment
fix continuation line
set required false
recommit
changed url for excluded products
uncommented get_success_url()
fix url
fix url
removed url2
fix success message
fixed error message
fix added msg
merged cleaned_query into clean
use add_error('query') in clean()
only check new products if there are
added RangeProductFileUpload.EXCLUDED_PRODUCTS_TYPE
moved validation to separate method
removed trailing whitespace
more trailing whitespace...
added comment
fix add product to excluded products
Establish better test base line
|
@gwaidacher Hey man. I have written the missing tests, so you don't have to do that anymore. |
Thank you very much :D |
We should thank you! You've spend quite some time into this. It'll likely be merged to master today! :) |
Added tabs to range - products in dashboard, first tab contains now existing form for managing included products, in the second tab the excluded products can be managed, with a separate textfield for upc input and a second fileupload form.
see #4055