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 Badge api #357

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

add Badge api #357

wants to merge 14 commits into from

Conversation

jhk482001
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Jan 17, 2025

Coverage Status

coverage: 81.799% (-1.3%) from 83.059%
when pulling 2fd6356 on badge-api
into 51658aa on master.

Copy link
Member

@nonumpa nonumpa left a comment

Choose a reason for hiding this comment

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

I would suggest squashing or removing duplicate commit messages.

This will make it easier for us to review the changes by commit and trace the history more effectively in the future.

docker-compose.yml Show resolved Hide resolved
mise.toml Outdated Show resolved Hide resolved
@jhk482001 jhk482001 force-pushed the badge-api branch 2 times, most recently from bdaae4f to 8431d58 Compare January 20, 2025 10:30
@jhk482001
Copy link
Collaborator Author

  1. clean commit - done
  2. rollback docker-compose.yml change
  3. rebase master branch
    Please help to review again @MrOrz @nonumpa

@MrOrz MrOrz requested review from MrOrz and nonumpa January 24, 2025 15:44
src/adm/handlers/moderation/__tests__/awardBadge.js Outdated Show resolved Hide resolved
src/graphql/dataLoaders/index.ts Outdated Show resolved Hide resolved
src/graphql/models/User.js Outdated Show resolved Hide resolved
src/graphql/models/User.js Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to putMapping in rumors-api. I have already re-indexed Elasticsearch DB on staging and production with the latest schema (by running npm run reload defined in rumors-db), so the users index already have badges field defined.

圖片

src/graphql/models/UserAwardedBadge.js Outdated Show resolved Hide resolved
src/graphql/models/Badge.js Outdated Show resolved Hide resolved

/* istanbul ignore if */
if (setbadgeIdResult === 'noop') {
console.log(`Info: user ID ${userId} already has set the same badgeId.`);
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the script above do not check if the same badge has been awarded or not, and always performs ctx._source.badges.add.
In this case, this console.log here would never be executed.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this unused loader implementation

Comment on lines +163 to +220
badges: {
type: new GraphQLNonNull(
new GraphQLList(new GraphQLNonNull(UserAwardedBadge))
),
description: 'Badges awarded to the user.',
resolve: (user) => user.badges || [],
},

majorBadgeBorderUrl: {
type: GraphQLString,
description: 'returns badge background image url',
resolve: async (user, args, { loaders }) => {
const displayItem = user.badges.find({ isDisplay: true });
if (displayItem == null) {
return null;
}
const badgeId = displayItem.id;
const badgeInfo = loaders.docLoader.load({
index: 'badges',
id: badgeId,
});
return badgeInfo.borderImage;
},
},

majorBadgeImageUrl: {
type: GraphQLString,
description: 'returns badge background image url',
resolve: async (user, args, { loaders }) => {
const displayItem = user.badges.find({ isDisplay: true });
if (displayItem == null) {
return null;
}
const badgeId = displayItem.id;
const badgeInfo = loaders.docLoader.load({
index: 'badges',
id: badgeId,
});
return badgeInfo.icon;
},
},

majorBadgeName: {
type: GraphQLString,
description: 'returns badge background image url',
resolve: async (user, args, { loaders }) => {
const displayItem = user.badges.find({ isDisplay: true });
if (displayItem == null) {
return null;
}
const badgeId = displayItem.id;
const badgeInfo = loaders.docLoader.load({
index: 'badges',
id: badgeId,
});
return badgeInfo.name;
},
},
Copy link
Member

Choose a reason for hiding this comment

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

It is recommended that we add a unit test (and relevant fixtures) under https://github.com/cofacts/rumors-api/blob/master/src/graphql/queries/__tests__/GetUser.js to ensure that these newly added fields can be loaded as expected.

@@ -39,7 +39,7 @@ services:
ELASTICSEARCH_URL: "http://db:9200"
URL_RESOLVER_URL: "url-resolver:4000"
ports:
- "5000:5000"
- "6000:5000"
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to ask: what is this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should revert this one, I found that Mac default use 5000 port for airplay receiver

Copy link
Member

Choose a reason for hiding this comment

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

This does not appear in the schema because this new query is not included in graphql/schema.js

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

4 participants