Skip to content

[6694][IMP] stock_valuation_fifo_lot: add validation error when there is no…#226

Open
AungKoKoLin1997 wants to merge 3 commits into
16.0from
16.0-fix-stock_valuation_fifo_lot_reval
Open

[6694][IMP] stock_valuation_fifo_lot: add validation error when there is no…#226
AungKoKoLin1997 wants to merge 3 commits into
16.0from
16.0-fix-stock_valuation_fifo_lot_reval

Conversation

@AungKoKoLin1997
Copy link
Copy Markdown
Contributor

@AungKoKoLin1997 AungKoKoLin1997 commented Apr 30, 2026

@nobuQuartile
Copy link
Copy Markdown
Contributor

I will follow up on adding the translation.

@nobuQuartile nobuQuartile changed the title [16.0][IMP] stock_valuation_fifo_lot: add validation error when there is no… [6694][IMP] stock_valuation_fifo_lot: add validation error when there is no… Apr 30, 2026
@yostashiro
Copy link
Copy Markdown
Member

Not sure if it's worth implementing this change. I guess the cause is rather obvious when the revaluation wizard silently not process anything. It's a corner-case which doesn't mess with data IIUC.

@nobuQuartile
Copy link
Copy Markdown
Contributor

Not sure if it's worth implementing this change. I guess the cause is rather obvious when the revaluation wizard silently not process anything. It's a corner-case which doesn't mess with data IIUC.

I think the user needs to clearly understand the situation, so showing an explicit error is preferable here.

Right now, the problematic behavior is that the user clicks the revaluation button in the wizard, the wizard simply closes without any error, but no stock valuation revaluation record is actually created. In that situation, the user cannot immediately tell whether the revaluation succeeded or not unless they refresh the page and carefully verify the valuation results afterward.

Technically, it is possible to notice the issue later by checking the valuation record, but from a usability perspective, raising an error at the moment of revaluation is much more user-friendly. It immediately tells the user that forced FIFO lot outbound processing is required, rather than requiring them to manually investigate the new valuation record.

I also agree this is probably not a frequent scenario since we have not encountered it before, but I still think the validation improves clarity and user experience enough to justify the change.

]
)
if not quants:
raise ValidationError(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As a matter of convention.

Suggested change
raise ValidationError(
raise UserError(

Copy link
Copy Markdown
Contributor

@nobuQuartile nobuQuartile left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants