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

Added note that user's email address may be shared with external orga… #956

Merged
merged 2 commits into from Mar 14, 2022

Conversation

Ankit-Gupta18
Copy link
Contributor

…nisation in application form for EMAIL partners

Description

Describe your changes in detail following the [commit message guidelines]
Added "If your application is successful we may share your email address with . This is required to set up your access to their resources. By submitting this form you agree to allow your email address to be shared with if your application is successful, for the purposes of account setup " at the top of forms.

Rationale

Phabricator Ticket

//: # https://phabricator.wikimedia.org/T300374

How Has This Been Tested?

Screenshots of your changes (if appropriate):

Types of changes

What types of changes does your code introduce? Add an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Minor change (fix a typo, add a translation tag, add section to README, etc.)

@Ankit-Gupta18
Copy link
Contributor Author

@suecarmol
Hi,
The work in this PR is still in progress. Kindly guide me through.

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
Please guide me through. It's been 10 days since I filed this PR.

@suecarmol
Copy link
Contributor

Hi @Ankit-Gupta18, I am in the process of refactoring most of the code here. Since this is not the right approach to solve this problem, I am going to wait until the refactoring is done so I can help you more.

@suecarmol suecarmol changed the title WEP - Added note that user's email address may be shared with external orga… WIP - Added note that user's email address may be shared with external orga… Feb 22, 2022
Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

Hi @Ankit-Gupta18. Thank you for your patience; I know that your PR took a long time to get a review. As I mentioned in the in-line comment, I don't think this is the correct approach. What you need to do is the following:

  1. Rebase your PR since there have been changes in the applications files.
  2. In the SubmitSingleApplicationView, you should add a get_context_data method. The method should look like this:
def get_context_data(self, **kwargs):
        context = super().get_context_data(**kwargs)

        partner = self._get_partner()
        context["partner"] = partner
        context["EMAIL"] = Partner.EMAIL

        return context

This should make all the information you need available in the template.
3. In the template (TWLight/applications/templates/applications/apply.html), you should add a condition. If partner has a EMAIL authorization_method, then that message should be displayed.

Let me know if you have any questions :)

TWLight/applications/forms.py Outdated Show resolved Hide resolved
@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol,
I am working on the changes requested by you. I am facing trouble adding a condition you mentioned in your 3rd point. Could you please detail me on it?

@suecarmol
Copy link
Contributor

Hi @suecarmol, I am working on the changes requested by you. I am facing trouble adding a condition you mentioned in your 3rd point. Could you please detail me on it?

Hi @Ankit-Gupta18, I was referring to an if template tag, found here: https://docs.djangoproject.com/en/3.2/ref/templates/builtins/#std:templatetag-if.

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol, I am working on the changes requested by you. I am facing trouble adding a condition you mentioned in your 3rd point. Could you please detail me on it?

Hi @Ankit-Gupta18, I was referring to an if template tag, found here: https://docs.djangoproject.com/en/3.2/ref/templates/builtins/#std:templatetag-if.

I have attached an image. Is this is what you are referring to?
ss7

@suecarmol
Copy link
Contributor

Hi @suecarmol, I am working on the changes requested by you. I am facing trouble adding a condition you mentioned in your 3rd point. Could you please detail me on it?

Hi @Ankit-Gupta18, I was referring to an if template tag, found here: https://docs.djangoproject.com/en/3.2/ref/templates/builtins/#std:templatetag-if.

I have attached an image. Is this is what you are referring to? ss7

Yes, that is the tag I am referring to, although the tag should be placed after the main-content div. Also, the condition is incorrect. You need to be checking if the partner's authorization method is equal to EMAIL. Please commit your changes so I can do a better review of your code.

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
I have addressed the reviews in a commit. Please check it.

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
I have addressed the final reviews in my recent commit.

@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
I just tried rebasing my PR, but in the end step, I am getting an error. Since this is the first time I am doing this, can you look into the screenshot attached with the error message, OR could rebase my PR?
rebase

@suecarmol
Copy link
Contributor

Hi @suecarmol , I just tried rebasing my PR, but in the end step, I am getting an error. Since this is the first time I am doing this, can you look into the screenshot attached with the error message, OR could rebase my PR? rebase

Try forcing the push with --force

@Ankit-Gupta18
Copy link
Contributor Author

Ankit-Gupta18 commented Mar 11, 2022

Hi @suecarmol , I just tried rebasing my PR, but in the end step, I am getting an error. Since this is the first time I am doing this, can you look into the screenshot attached with the error message, OR could rebase my PR? rebase

Try forcing the push with --force

Hi @suecarmol
Still the same result. I have searched for all possible solutions but nothing worked. Can you rebase it?

Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

I have rebased your PR. Just one more thing and this will be ready to merge.

TWLight/applications/templates/applications/apply.html Outdated Show resolved Hide resolved
TWLight/applications/templates/applications/apply.html Outdated Show resolved Hide resolved
@Ankit-Gupta18
Copy link
Contributor Author

Hi @suecarmol ,
I have made the final changes. Also thanks for your help so far.

@suecarmol suecarmol changed the title WIP - Added note that user's email address may be shared with external orga… Added note that user's email address may be shared with external orga… Mar 12, 2022
…nisation in application form for EMAIL partners

Addressed reviews

Addressed final reviews
Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

Thank you for your patience and for completing this task.

@suecarmol suecarmol merged commit 27452cc into WikipediaLibrary:master Mar 14, 2022
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

2 participants