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

Implement lending system #3501

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Implement lending system #3501

wants to merge 30 commits into from

Conversation

Arashfa0301
Copy link
Contributor

@Arashfa0301 Arashfa0301 commented Oct 4, 2023

Resolves ABA-613

@Arashfa0301 Arashfa0301 added backend do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged labels Oct 4, 2023
Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

We need an image field in lendableObject, and responsibleRole needs to support multiple roles (and should then be called responsibleRoles)

@linear
Copy link

linear bot commented Oct 20, 2023

ABA-613 Lending system

Backup has requested a lending system, as Abakus has a lot of objects that could be lent out to Ababeads if we just had a good system for it. Arrkom also wants this system to make it easier to administrate Soundboks and grill lending.

@eikhr eikhr marked this pull request as draft October 20, 2023 10:59
…mplement-lending-system

# Conflicts:
#	lego/apps/lending/managers.py
#	lego/apps/lending/models.py
#	lego/apps/lending/serializers.py
#	lego/apps/lending/views.py
@jonasdeluna jonasdeluna marked this pull request as ready for review April 23, 2024 17:16
@jonasdeluna jonasdeluna changed the title Draft: Implement lending system Implement lending system Apr 23, 2024
lego/api/v1.py Show resolved Hide resolved
lego/apps/lending/notifications.py Outdated Show resolved Hide resolved
lego/apps/lending/managers.py Outdated Show resolved Hide resolved
lego/apps/lending/managers.py Outdated Show resolved Hide resolved
lego/apps/lending/managers.py Outdated Show resolved Hide resolved
@jonasdeluna
Copy link
Member

@eikhr I'll look closer at the comments after the 2 upcoming exams 👍👍

Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 83.94794% with 148 lines in your changes are missing coverage. Please review.

Project coverage is 88.73%. Comparing base (6abb9ef) to head (cd58144).
Report is 97 commits behind head on master.

Files Patch % Lines
lego/apps/lending/permissions.py 32.75% 39 Missing ⚠️
lego/apps/lending/serializers.py 57.69% 22 Missing ⚠️
lego/apps/forums/views.py 68.75% 20 Missing ⚠️
lego/apps/lending/managers.py 0.00% 18 Missing ⚠️
lego/apps/lending/views.py 65.21% 16 Missing ⚠️
lego/apps/forums/permissions.py 73.07% 14 Missing ⚠️
lego/apps/lending/notifications.py 0.00% 14 Missing ⚠️
lego/apps/lending/models.py 95.00% 2 Missing ⚠️
lego/apps/lending/validators.py 50.00% 2 Missing ⚠️
lego/apps/joblistings/views.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3501      +/-   ##
==========================================
+ Coverage   88.20%   88.73%   +0.53%     
==========================================
  Files         666      696      +30     
  Lines       21069    21726     +657     
==========================================
+ Hits        18584    19279     +695     
+ Misses       2485     2447      -38     

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

@jonasdeluna
Copy link
Member

@eikhr I've fixed your comments, notifications still don't work so they are removed for now. Will squash when merged.

Copy link
Member

@eikhr eikhr 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 something weird has happened during a rebase maybe? A lot of unrelated changes in the diff

Comment on lines +39 to +57
def get_reactions_grouped(self, user):
grouped = {}
for reaction in self.reactions.all():
if reaction.emoji.pk not in grouped:
grouped[reaction.emoji.pk] = {
"emoji": reaction.emoji.pk,
"unicode_string": reaction.emoji.unicode_string,
"count": 0,
"has_reacted": False,
"reaction_id": None,
}

grouped[reaction.emoji.pk]["count"] += 1

if reaction.created_by == user:
grouped[reaction.emoji.pk]["has_reacted"] = True
grouped[reaction.emoji.pk]["reaction_id"] = reaction.id

return sorted(grouped.values(), key=lambda kv: kv["count"], reverse=True)
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here? Why is this in this PR?

@@ -44,6 +46,33 @@ class Meta:
"additional_emails",
)

def validate(self, attrs: Any) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

And this stuff has nothing to do with the lending system

@jonasdeluna
Copy link
Member

I think something weird has happened during a rebase maybe? A lot of unrelated changes in the diff

Hmm actually I merged instead of rebasing, how can I resolve it now?

@eikhr
Copy link
Member

eikhr commented May 28, 2024

I think something weird has happened during a rebase maybe? A lot of unrelated changes in the diff

Hmm actually I merged instead of rebasing, how can I resolve it now?

I don't know, I haven't seen this before.
Maybe a rebase will help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants