Skip to content

Gallery Organizers#85

Merged
RamZallan merged 1 commit intoComputerScienceHouse:developfrom
jabbate19:develop
Oct 20, 2021
Merged

Gallery Organizers#85
RamZallan merged 1 commit intoComputerScienceHouse:developfrom
jabbate19:develop

Conversation

@jabbate19
Copy link
Copy Markdown
Contributor

@jabbate19 jabbate19 commented Oct 20, 2021

Fixes #82

Hacktober Pls :)

@RamZallan
Copy link
Copy Markdown
Member

For future reference, there's some wording (documented here) that you can use (vs just mentioning the issue number) that'll automatically close the issue once the associated PR is merged.

i.e. you can edit the main description of this PR to say something like "Fixes #82"

Comment thread gallery/__init__.py
app.config.get("EBOARD_UIDS", "").split(","),
app.config.get("RTP_UIDS", "").split(","),
app.config.get("ORGANIZER_UIDS", "").split(","),
app.config.get("ALUMNI_UIDS", "").split(","),
Copy link
Copy Markdown
Member

@RamZallan RamZallan Oct 20, 2021

Choose a reason for hiding this comment

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

ALUMNI_UIDS doesn't seem to be used anywhere else in your PR, maybe remove it? Unless it's WIP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This gets used by the ldap mock, which makes sense to me since alums have different privileges than active members.

That being said, are these documented/added to config samples?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh that makes sense, thanks. Doesn't look like it -- @jabbate19 could you add them to the config samples for clarity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I added because I realized there was nothing set up for ldap mock on alumni, so when I ran it in dev it failed bc it couldn't find the actual LDAP.

Copy link
Copy Markdown
Member

@RamZallan RamZallan left a comment

Choose a reason for hiding this comment

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

Besides this + the config sample note, this all looks good to me! Has it been tested locally? (w/ the LDAP mock, I assume?)

Comment thread gallery/ldap.py Outdated
self._ldap = ldap
self._eboard: List[str] = []
self._rtp: List[str] = []
self._organzier: List[str] = []
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

organizer is misspelled here

@jabbate19 jabbate19 requested a review from RamZallan October 20, 2021 19:33
@RamZallan
Copy link
Copy Markdown
Member

RamZallan commented Oct 20, 2021

Before I merge, @jabbate19 could you squash your commits into one? (HMU on Slack if you don't know how and I can help)

Fix Gallery Organizers

Fix Finalized Organizer

Remove Files

Re-add things I touched from dev

Add to sample, fix spelling
@RamZallan RamZallan merged commit e274a98 into ComputerScienceHouse:develop Oct 20, 2021
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.

Create Functionality for Organizer Role

3 participants