#144 Add ability to see the current user's sent and received packets
Merged by jflory7. Opened by shraddhaag.
fedora-commops/ shraddhaag/fedora-happiness-packets functionality-to-view-sent-and-received-messages  into  master

Download 144.patch

Q - What is a summary of your change?
A - The PR adds ability for the current logged in user to see their sent and received packets. Solves issue #43

Q - Why is this change helpful?
A - It adds richness to the overall UX of the user. This helps keep the user a score of their activity and see their received and sent packets without opening their mails.

Q - Any specific details to consider?
A - All the details of the changes made can be found in ticket #43 and the commit messages.

This is abstract thought, but the code across these two methods is more or less identical. I wonder if there is a better way to use inheritance to write a generic class that both the ReceivedMessagesView and SentMessagesView inherit from. However, since these methods are so small, I don't think it's a big issue if it makes things more complicated than it is worth.

This is definitely a nitpick edit. But I think the second sentence (Why not start the chain of appreciation at home?) is confusing from an English point-of-view. I suggest dropping the at home part of the sentence.

I always feel bad for nitpick edits. :scream: But this is also a tiny catch, the Lets should be written as Let's since it is meant as a contraction. A better explanation of this is given here.

Nice idea to pull this out. :thumbsup:

@shraddhaag Excellent work. :100: This is awesome! I tested it locally and almost everything worked as expected, except for one thing.

I noticed a message appeared in the My Received Packets page before the packet was confirmed in the sender's email. A packet should only display there once the sender has confirmed to send the packet via email. Is there a flag set anywhere once a packet is confirmed by the sender?

Also, I think there is a caveat specific to Fedora. Currently, it looks like the email used to match someone's received packets is the email address set in the Fedora Account System, instead of a Fedora Project email alias. For example, I see packets sent to jflory7 [at] gmail [dot] com, but I do not see packets sent to jflory7 [at] fedoraproject [dot] org. One possible solution is to to check for two email addresses. It can be the person's actual email, as provided by FAS, and then an email built off of their FAS username at the fedoraproject.org domain. So for me, that means I would see both packets sent to my Gmail email and my fedoraproject.org email alias.

Also, for the fedoraproject.org email alias, by default it is always generated from someone's FAS username. Some edge cases exist here and there (for example, I also have jwf [at] fedoraproject [dot] org), but I think it's okay to accept this as an edge case that is not in scope.

Metadata Update from @jflory7:
- Pull-request tagged with: PASSED, improvement, needs changes, type - backend, type - frontend, type - summer coding
- Request assigned

2 new commits added

  • Implement DRY in pagination
  • Add ability to see my sent and received packets

@jflory7 I have implemented all the requested changes.
1. Made a base class UserMessageView that the other two classes inherit from
2. Corrected both the errors in place holder text. (I'll make sure to not push such errors in future :sweat_smile: )
3. Corrected the check at the receiver's end. This skipped my head completely! Thanks for mentioning this one.
4. Messages on both emails can now be seen in My received Packets

Please let me know if there's something else needed or missing! :)

2 new commits added

  • Implement DRY in pagination and refactor code in views
  • Add ability to see my sent and received packets

@jflory7 I refactored the code in the views.py further by:
1. Making the base class handle the query filter that is common to both child classes.
2. Base class handles the login required check as it is required in both the views.

@shraddhaag Awesome, thanks for addressing the feedback so quickly! I'll try to re-test these changes by Friday or sooner.

Metadata Update from @jflory7:
- Pull-request untagged with: needs changes
- Pull-request tagged with: needs testing

Ooh, nice! :thumbsup: I learned something new here too.

Oops, slight typo here. Recieved should be Received.

recieved => received

@shraddhaag Super work! :100: I just tested again. Everything works as expected. Your changes got all of my feedback.

Before merging, I caught a couple of very minor typos. When you have time, please fix those. Then this PR is ready to go. Please squash your commits down to one commit once you're done and we'll merge it. :confetti_ball:

rebased onto 4f77220aec05a74bb4b2c74c0874bd4134acc543

@jflory7 Thank you! :smile:

I have made the changes. Rest assured, I have activated this to avoid such embarrassing mistakes in future :sweat_smile:

Squashed everything to 1 commit. Hope everything works out now! :)

I have made the changes. Rest assured, I have activated this to avoid such embarrassing mistakes in future

No worries, it happens. I've done it many times before too. :sweat_smile:

Squashed everything to 1 commit. Hope everything works out now! :)

Awesome! Thanks @shraddhaag. Merging. :clapper:

Pull-Request has been merged by jflory7