Skip to content

fix: Removed the option to add an item through barcode from Add a Price page #6525

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Abhishek-P0207
Copy link
Contributor

What

Fixed the issue of adding an unrelated item through barcode, from "Add a price" page of a specific product.

Screenshot

Before:

After:

Fixes bug(s)

Part of

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2025

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 5.84%. Comparing base (4d9c7fc) to head (d0e14b0).
Report is 837 commits behind head on develop.

Files with missing lines Patch % Lines
...kages/smooth_app/lib/pages/prices/price_model.dart 0.00% 2 Missing ⚠️
...h_app/lib/pages/prices/product_price_add_page.dart 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #6525      +/-   ##
==========================================
- Coverage     9.54%   5.84%   -3.71%     
==========================================
  Files          325     499     +174     
  Lines        16411   29827   +13416     
==========================================
+ Hits          1567    1744     +177     
- Misses       14844   28083   +13239     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @Abhishek-P0207!
Correct me if I'm wrong, but you've just removed the "add a product" block altogether, right?
That's not what was supposed to be done:

  • there are cases when we land on that page from a single product page, and in that case only we shouldn't be able to add products
  • there are cases when we land on that page from a "add prices from receipt" button, and in that case we should definitely be able to add products (especially as we start with an empty list of products)

@Abhishek-P0207
Copy link
Contributor Author

Hmm , I will fix it.

@Abhishek-P0207
Copy link
Contributor Author

@monsieurtanuki I fixed it, but I have a doubt regarding the "Add price tags" button in the prices section.
Both "Add a receipt" and "Add price tags" go to the same page, then why separate buttons, also in the adding section we provide a radio button to switch between them.We can combine them in one single button right?

@Abhishek-P0207
Copy link
Contributor Author

The "Add price tags" should not have option of Add an item right?
As, it is not possible to have a list of items like that of a receipt, in the form of a price tag. The receipt can contain bill for single item or multiple items right? But price tag will only contain a single item. We can add the proof of receipt, and add multiple items but i don't know how Add price tags work!!
Can you clarify this @monsieurtanuki

@monsieurtanuki
Copy link
Contributor

I fixed it

You did? Where's the code?

but I have a doubt regarding the "Add price tags" button in the prices section. Both "Add a receipt" and "Add price tags" go to the same page

That's correct

then why separate buttons, also in the adding section we provide a radio button to switch between them.We can combine them in one single button right?

Don't worry, keep it that way.

@monsieurtanuki
Copy link
Contributor

The "Add price tags" should not have option of Add an item right?

Yes it should
image

@Abhishek-P0207
Copy link
Contributor Author

@monsieurtanuki The issue is fixed, please review it.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @Abhishek-P0207!
Unfortunately your solution doesn't work.
It should NOT depend on the current status (what if I switch from receipt to price tag? what if I start with an empty list and add one product?).
The solution is in the model: "hey, just don't show an 'add product' button". And that bool could be set as a final in the model creation, depending on where you opened the "add price" page.

@Abhishek-P0207
Copy link
Contributor Author

Hmm, Interesting. I will look into it. As Lab exams are going on now, I am not able to contribute in a continuous basis. exams will be over by day after tmrw, so can i contribute from then on? I will try to fix this if I find any free time in between.
Thank you

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @Abhishek-P0207, and thank you for your code rewriting!
Please have a look at my comments.

Additional comments:

  • I would have named the field "singleProduct" instead of "multipleProducts" (obviously with the opposite purpose), but it's just a matter of taste
  • generally speaking it's not such a good idea to use default values when the default choice is not that obvious. More specifically here, without the default value the developer is explicitly asked "is that a single product model?", and it's a fair question to ask.

@@ -19,12 +19,14 @@ class PriceModel with ChangeNotifier {
required final List<OsmLocation>? locations,
required final Currency currency,
final PriceMetaProduct? initialProduct,
final bool? multipleProducts,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final bool? multipleProducts,
required this.multipleProducts,

}) : _proof = null,
existingPrices = null,
_proofType = proofType,
_date = DateTime.now(),
_currency = currency,
_locations = locations,
_multipleProducts = multipleProducts ?? false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_multipleProducts = multipleProducts ?? false,

Comment on lines +55 to +62
late bool _multipleProducts;
bool showAddProductCard() {
if (_multipleProducts == true) {
return true;
}
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
late bool _multipleProducts;
bool showAddProductCard() {
if (_multipleProducts == true) {
return true;
}
return false;
}
/// "Should we support multiple products?" (instead of a single)
final bool multipleProducts;

For the record, your previous syntax was a bit heavy:

  bool showAddProductCard() {
    if (_multipleProducts == true) {
      return true;
    }
    return false;
  }

The following would have been better:

  bool get showAddProductCard => _multipleProducts;

@@ -65,6 +66,7 @@ class ProductPriceAddPage extends StatefulWidget {
locations: osmLocations,
initialProduct: product,
currency: currency,
multipleProducts: multiProduct,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
multipleProducts: multiProduct,
multipleProducts: multipleProducts,

Please try to keep consistency between classes.

@@ -152,7 +154,7 @@ class _ProductPriceAddPageState extends State<ProductPriceAddPage>
index: i,
),
const SizedBox(height: LARGE_SPACE),
const PriceAddProductCard(),
if (model.showAddProductCard()) const PriceAddProductCard(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (model.showAddProductCard()) const PriceAddProductCard(),
if (model.multipleProducts) const PriceAddProductCard(),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Status: 💬 To discuss and validate
Development

Successfully merging this pull request may close these issues.

You shouldn't be able to add an unrelated price from the page of a product
3 participants