Hi @jflory7 can you please look into this when you have time? :smile:
Eagerly waiting for your feedback :)

PS: I'm sorry about not making it an incremental upgrade, I had too many commits during development thus I squashed them all together into one. I sincerely apologise if this makes it hard to review.

@shraddhaag Thanks for this PR!! Super exciting to see this work. I won't have time for a detailed review until later, but I'll try to round up a couple of others more familiar with fedora-messaging to take a look at your changes.

@jcline @abompard @cverna If any of you have time for a fedmsg => fedora-messaging PR review, your insight here is super helpful. :grinning:

This is the default behavior of the parent class[0] so you should be able to drop this property.

[0] https://github.com/fedora-infra/fedora-messaging/blob/129cfb997046a7c1b973e1c81953d4d812d9b5bf/fedora_messaging/message.py#L610

The id needs to be unique. I think "https://fedoraproject.org/message-schema/happiness_packet" would be fine if this is the only message the app sends.

Minor nitpik, but this indentation should line up with "properties"

This is the name of the Python package of the schema, so it'd probably be best to call it "happinesspacket_messages" or "fedora_happinesspacket_messages" so it's clear it's not the happinesspacket application itself.

Don't forget to update the URL to this repo :grinning:

This call can raise a couple different exceptions[0] that you'll want to handle here. Since this is happening in a HTTP endpoint, handling them all should be pretty straightforward.

1. fedora_messaging.exceptions.PublishReturned - This exception occurs when the broker is up, but for some reason cannot accept the message (no more disk space, for example). The best thing to do here is probably send the client a HTTP 503 Service Unavailable[1] since the broker is in trouble.

2. fedora_messaging.exceptions.ConnectionException - This happens if we can't even connect to the broker after the configured number of connection attempts. It might be being rebooted, or there's a network partition, but at any rate we can't hang around forever waiting so we should again send a HTTP 503 to inform the user to try again later.

For both these, if this is a user-facing page (I don't know much about this app) you should be able to make some sort of HTML page that tells the user more than the generic HTTP 503 page. Something like "The message could not be sent because {{ reason }}, please try again later!".

The final exception, fedora_messaging.exceptions.ValidationError, is not one you need to handle. It's there to let the developer (you) know if they're sending a message that isn't valid.

[0] https://fedora-messaging.readthedocs.io/en/stable/api.html#fedora_messaging.api.publish
[1] https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

You'll probably not want to pin this exact version. "fedora-messaging>=1.4.0" should be fine.

I can't comment inline on the binary files, but you don't want to check in the files from dist/ (Happinesspacket-1.0.0-py2.py3-none-any.whl and the tar.gz). Those are build artifacts generated with "python setup.py sdist bdist_wheel".

Hi @jcline
Thank you for the detailed review! :)
I've integrated all the changes you asked.
Could you please find the time to review the changes?

I've also uploaded the package on PyPI which can be found here with all the changes integrated.
I'm working on submitting an RPM package for it to Fedora and EPEL.

A completely equivalent way to write this if/else would be:

sender_name = self.request.user.username if message.sender_name else "Anonymous"

It's totally personal taste for style, and I have no opinion, but just in case you didn't know.

Since this is only used later in the construction of the MessageV1 class, it'd probably be better to just do "recipient": message.recipient_name when you make the message. While there's nothing wrong with this code, it's not necessary and made me look around for where the recipient name got modified.

The Python namespace you're using is "happinesspacket_schema", so it would be best to use that name here. I assume this works, but it's not common for the name of the package to not match the Python namespace.

There's actually a (probably not well-documented) version of this that ships with fedora-messaging in the "fedora_messaging.example" module. You should be able to run it with the CLI fedora-messaging ships with by running "fedora-messaging consume --callback=fedora_messaging.example:printer". You can specify the configuration with the --conf flag or with the FEDORA_MESSAGING_CONF environment variable.

@shraddhaag , looks good :sparkles: :cake:

I left a few minor notes, and I've not actually tested the code, but it generally looks right to me. @jflory7, back to you!

@jcline Thank you for the review again! :)
I've made the changes as asked. The package on PyPI can be found here.

@jflory7 Super excited for the review :)
I'm working on updating the documentation meanwhile.

Task left to be done:
Currently the name of the recipient is send to the broker. Instead, the username of the recipient should be sent as that will be needed to assign them the badge.
Configure a way to get the username of the recipient from the email input by the user. [WIP]

As an afterthought, this could be done after #127 is completed. If using FAS to search for accounts, it will be easier to get the recipient's username.

@shraddhaag, do you want to open a new ticket to track this?

@jflory7 Super excited for the review :)
I'm working on updating the documentation meanwhile.

I just tested this out locally and it works exactly as described. :tada: :tada: This is so awesome, I wasn't expecting for this to be done so quickly. Next week, we'll have to see if we can get this pushed to staging. :smile:

Thanks again @shraddhaag, merging! :clapper:

As an afterthought, this could be done after #127 is completed. If using FAS to search for accounts, it will be easier to get the recipient's username.
@shraddhaag, do you want to open a new ticket to track this?

I have been tracking that issue for the same reason and trying to figure out a way myself for it.
I'll open an issue so that it's easier to track things and keep everyone in the loop.

I just tested this out locally and it works exactly as described. 🎉 🎉 This is so awesome, I wasn't expecting for this to be done so quickly. Next week, we'll have to see if we can get this pushed to staging. 😄
Thanks again @shraddhaag, merging! 🎬

Thank you so much @jflory7 Extremely glad I could get this done :)
I'm on board for staging :)