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.

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

Add named IDs to stregsystemet #295

Merged
merged 23 commits into from
May 11, 2022
Merged

Conversation

Zaph-x
Copy link
Member

@Zaph-x Zaph-x commented Feb 11, 2022

This pull request adds named IDs to stregsystemet.
This has been requested by several fembers since STS got named IDs as shorthands for buying.
The named IDs do not change the way products are parsed when purchases, as they are handled in pre-processing. This means the user does not need to know about the IDs, however may use them if they are known to the user.
An example of a named ID purchase is tester øl which would be preprocessed to tester 14 by looking for a named ID in the database linked to øl. If an ID is not found, the string is left as is (e.g. tester asdfghjkl would remain the same) and give a parse error.

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2022

Codecov Report

Merging #295 (7e40718) into next (2b8ac75) will increase coverage by 0.39%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##             next     #295      +/-   ##
==========================================
+ Coverage   82.44%   82.84%   +0.39%     
==========================================
  Files          31       31              
  Lines        2518     2623     +105     
  Branches      182      188       +6     
==========================================
+ Hits         2076     2173      +97     
- Misses        412      418       +6     
- Partials       30       32       +2     
Impacted Files Coverage Δ
stregsystem/templatetags/stregsystem_extras.py 89.28% <72.72%> (-10.72%) ⬇️
stregsystem/views.py 56.71% <75.00%> (+0.81%) ⬆️
stregsystem/models.py 88.18% <88.88%> (+0.01%) ⬆️
stregsystem/admin.py 74.22% <100.00%> (+0.82%) ⬆️
stregsystem/tests.py 99.79% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b8ac75...7e40718. Read the comment docs.

Copy link
Member

@hrjakobsen hrjakobsen left a comment

Choose a reason for hiding this comment

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

I haven't tested this, only read the code changes, and left some comments based on that. I think some more validation of the names of NamedProduct are required.

pyproject.toml Outdated Show resolved Hide resolved
stregreport/urls.py Outdated Show resolved Hide resolved
stregsystem/models.py Outdated Show resolved Hide resolved
stregsystem/urls.py Outdated Show resolved Hide resolved
stregsystem/views.py Show resolved Hide resolved
stregsystem/views.py Show resolved Hide resolved
treo/settings.py Outdated Show resolved Hide resolved
treo/urls.py Outdated Show resolved Hide resolved
stregsystem/models.py Outdated Show resolved Hide resolved
stregsystem/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@joandrsn joandrsn left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@hrjakobsen hrjakobsen left a comment

Choose a reason for hiding this comment

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

I think alias handling in views could be handled better.

  • Why do we show a random alias instead of all?
  • Why do we have to show an alias if none exist? (N/A -> id)

However this is nothing that blocks merging.

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.

None yet

6 participants