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

Fixes #790: API now generates badges #791

Merged
merged 1 commit into from May 19, 2018

Conversation

gabru-md
Copy link
Member

@gabru-md gabru-md commented May 18, 2018

Fixes #790

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • I have added necessary documentation (if appropriate)

Preview Link

Changes proposed in this pull request:

  • This PR contains the code for generating badges from the new API.

Thank You!

@gabru-md
Copy link
Member Author

@harshit98 @djmgit @ParthS007 @yashLadha
@vaibhavsingh97 review please.

@open-event-bot
Copy link

Hi @gabru-md!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot
Copy link

Hi @gabru-md!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot
Copy link

Hi @gabru-md!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot
Copy link

Hi @gabru-md!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot
Copy link

Hi @gabru-md!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@gabru-md
Copy link
Member Author

gabru-md commented May 18, 2018

Here is a video for you to review.
Video Preview
Thanks

@codecov
Copy link

codecov bot commented May 18, 2018

Codecov Report

Merging #791 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           development   #791   +/-   ##
==========================================
  Coverage          100%   100%           
==========================================
  Files                1      1           
  Lines               43     43           
==========================================
  Hits                43     43

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 f550271...7653eb8. Read the comment docs.

from api.utils.response import Response
from api.utils.svg_to_png import SVG2PNG
from api.utils.merge_badges import MergeBadges


router = Blueprint('generateBadges', __name__)

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 extra line

@open-event-bot
Copy link

Hi @gabru-md!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@gabru-md
Copy link
Member Author

@djmgit the change you have suggested is failing the build. So I am reverting it.
I guess it is okay? 😄

'Could not find any JSON'))

csv_name = data['csv']
image_name = data['image']
Copy link
Member

Choose a reason for hiding this comment

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

use data.get() format to avoid exception

str(e),
'Could not find any JSON'))

csv_name = data['csv']
Copy link
Member

Choose a reason for hiding this comment

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

please check for the availability of minimum required parameters and raise required error responses

@@ -4,8 +4,8 @@
class File(db.Model):
__tablename__ = 'File'

filename = db.Column(db.String(100), NULLABLE=False)
filetype = db.Coloumn(db.string(100), NULLABLE=False)
filename = db.Column(db.String(100), nullable=False, primary_key=True)
Copy link
Member

Choose a reason for hiding this comment

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

How can you set filename as primary key? How are you making sure that it will be unique?

Copy link
Member Author

@gabru-md gabru-md May 18, 2018

Choose a reason for hiding this comment

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

uuid.uuid4 and uuid.uuid1 provide unique names always.
I already had it discussed with @yashLadha and @ParthS007 since they raised the same doubt

Copy link
Member

Choose a reason for hiding this comment

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

okay

Copy link
Member Author

Choose a reason for hiding this comment

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

and we are using uuid.uuid4 for generating the names. therefore it will always be unique.

Copy link
Member

@djmgit djmgit left a comment

Choose a reason for hiding this comment

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

Please see comments inline

@gabru-md
Copy link
Member Author

@djmgit please review again. I've made the changes.

This PR has added generating badges functionality implemented from scratch.

Cheers!

removed my config

Fixes Travis

fix

modifies .gitignore foe firebase

Adds fix for travis

Fixes Coadcy Errors

Removes Extra Line

revert

Made Changes
Copy link
Member

@djmgit djmgit left a comment

Choose a reason for hiding this comment

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

@gabru-md what about the manually entered data? I mean when no csv is there, people can enter data manually right? What about that. We also need a parameter for that.

@gabru-md
Copy link
Member Author

@djmgit for the manual data we will first save it into a CSV and then apply the same method for generation of badges.
Why save to CSV first?
Since we have to make a Badge Management Portal, we will need to store that input somewhere or the other. So Saving that maunal input as a CSV and then following the same above steps will ensure that the API works properly for us in present and as well as the future.
For saving the manual input we will require an extra route, since we are making separate components on the frontend to deal with separate things, therefore we require separate api routes to collect the data from them as well, just like in case of open event server.

I am creating an issue for the same and will send a PR by the end of the day. In this way, the current proposed badge making functionality will remain isolated from the fact that the input was manual or csv. As we can see in the previous version it caused a lot of ifs and elses which made the code less maintainable as well. Meanwhile this can be merged, and I will work on accepting the manual data on a separate route.

Please merge this @djmgit
thank you 😃

@djmgit djmgit merged commit 14a3830 into fossasia:development May 19, 2018
@gabru-md gabru-md deleted the generate_badges branch May 19, 2018 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adding generate badge functionality
4 participants